Repository navigation
Show merged PR status in thread sidebar when no open PR exists - #53
Closed
juliusmarminge wants to merge 7 commits into
Closed
juliusmarminge wants to merge 7 commits into
juliusmarminge wants to merge 7 commits into
Conversation
- extend `GitStatusResult` with nullable `mergedPr` metadata - in server git status, query merged PR only when no open PR is found - in sidebar, display a `Merged` status pill for threads on branches with merged PRs - update server/web/contracts tests for the new status shape
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces Changes
Sequence DiagramsequenceDiagram
participant Web as Sidebar (Web)
participant API as GitManager (Server)
participant GH as GitHub CLI
Web->>API: status(cwd, branch)
API->>GH: gh pr list --state all --json number,title,url,baseRefName,headRefName,state,updatedAt,mergedAt
GH-->>API: list (may be empty)
alt list non-empty
API->>API: parsePullRequestList -> normalize state/updatedAt
API->>API: sort by updatedAt -> pick latest PR
API-->>Web: GitStatusResult { pr: latestPR, ... }
else empty
API-->>Web: GitStatusResult { pr: null, ... }
end
alt pr.state == "merged"
Web->>Web: render "PR merged" pill
else pr.state == "open"
Web->>Web: render "PR open" pill and link
else
Web->>Web: no PR pill
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Contributor
Show per-thread PR state in the Sidebar and return
|
Greptile SummaryThis PR adds merged PR detection to the git status flow. When no open PR exists for a branch, the server now falls back to querying
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Sidebar as Sidebar (useQueries)
participant WS as WebSocket Server
participant GM as GitManager.status()
participant GH as GitHub CLI (gh)
Sidebar->>WS: git.status({ cwd })
WS->>GM: status({ cwd })
GM->>GM: gitCore.statusDetails(cwd)
alt branch exists
GM->>GH: gh pr list --head branch --state open
alt open PR found
GH-->>GM: [{ number, title, url, ... }]
GM-->>WS: { openPr: {...}, mergedPr: null }
else no open PR
GH-->>GM: []
GM->>GH: gh pr list --head branch --state merged
alt merged PR found
GH-->>GM: [{ number, title, url, ... }]
GM-->>WS: { openPr: null, mergedPr: {...} }
else no merged PR
GH-->>GM: []
GM-->>WS: { openPr: null, mergedPr: null }
end
end
else no branch (detached HEAD)
GM-->>WS: { openPr: null, mergedPr: null }
end
WS-->>Sidebar: GitStatusResult
Sidebar->>Sidebar: Derive mergedPrByThreadId
Sidebar->>Sidebar: Show "Merged" pill if applicable
Last reviewed commit: 46347e2 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let openPr: ReturnType<typeof toStatusOpenPr> | null = null; | ||
| if (details.branch && details.hasUpstream) { | ||
| let mergedPr: ReturnType<typeof toStatusOpenPr> | null = null; | ||
| if (details.branch) { |
There was a problem hiding this comment.
hasUpstream guard removed for findOpenPr too
The old condition was if (details.branch && details.hasUpstream), which avoided calling gh pr list for branches that have never been pushed to a remote. Removing hasUpstream is necessary for the merged PR path (the remote branch may have been deleted after merge), but it also means findOpenPr now runs for local-only branches that can't possibly have an open PR. This adds an unnecessary gh CLI call (with 30s timeout) on every status check for unpushed branches.
Consider restoring the hasUpstream check for findOpenPr and only skipping it for the merged lookup:
Suggested change
| if (details.branch) { | |
| if (details.branch) { | |
| try { | |
| if (details.hasUpstream) { | |
| const existing = await this.findOpenPr(input.cwd, details.branch); | |
| if (existing) { | |
| openPr = toStatusOpenPr(existing); | |
| } | |
| } | |
| if (!openPr) { | |
| const merged = await this.findMergedPr(input.cwd, details.branch); | |
| if (merged) { | |
| mergedPr = toStatusOpenPr(merged); | |
| } | |
| } |
Comment on lines
+156
to
+166
| const threadGitStatusQueries = useQueries({ | ||
| queries: threadGitTargets.map((target) => { | ||
| const base = gitStatusQueryOptions(api, target.cwd); | ||
| return { | ||
| ...base, | ||
| enabled: Boolean(base.enabled && target.branch !== null), | ||
| staleTime: 30_000, | ||
| refetchInterval: 60_000, | ||
| }; | ||
| }), | ||
| }); |
There was a problem hiding this comment.
Per-thread git status polling may cause performance issues
This fires a separate useQueries entry for every thread. Although queries with the same cwd will share a cache key, threads using worktrees each have a unique path, meaning N worktree threads = N independent status queries. Each query triggers git status --porcelain + up to 2 gh pr list calls (open + merged), polling every 60s. For users with many worktree threads, this could saturate gh API rate limits or slow down the sidebar.
Consider deduplicating by cwd before passing to useQueries, or only querying for visible/expanded threads.
| for (let index = 0; index < threadGitTargets.length; index += 1) { | ||
| const target = threadGitTargets[index]; | ||
| if (!target) continue; | ||
| const status = threadGitStatusQueries[index]?.data as GitStatusResult | undefined; |
There was a problem hiding this comment.
Unsafe as type assertion on query data
threadGitStatusQueries[index]?.data is typed as the inferred return type from the query function, but using as GitStatusResult | undefined bypasses type safety. If the query's return type ever diverges from GitStatusResult, this cast will silently hide the mismatch. Consider using a type-safe approach, e.g., assigning the result of gitStatusQueryOptions with an explicit generic or using a type guard.
- server: only query open PRs when the current branch has an upstream; still fall back to merged PR lookup - tests: cover status behavior for branches without upstream tracking - web: fetch git status once per cwd, validate query data with schema parsing, and map merged PR badges by thread branch match
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/server/src/gitManager.ts (1)
421-447: Consider extracting a shared helper to reduce duplication.
findMergedPrandfindOpenPrare nearly identical, differing only in the--stateargument. This could be consolidated into a single method with a state parameter.♻️ Optional refactor to reduce duplication
- private async findOpenPr(cwd: string, branch: string): Promise<OpenPrInfo | null> { + private async findPrByState( + cwd: string, + branch: string, + state: "open" | "merged", + ): Promise<OpenPrInfo | null> { const stdout = await this.runGhStdout(cwd, [ "pr", "list", "--head", branch, "--state", - "open", + state, "--limit", "1", "--json", "number,title,url,baseRefName,headRefName", ]); const raw = trimStdout(stdout); if (raw.length === 0) return null; let parsedJson: unknown; try { parsedJson = JSON.parse(raw); } catch { throw new Error("GitHub CLI returned invalid PR list JSON."); } const parsed = parseOpenPrList(parsedJson); return parsed[0] ?? null; } - private async findMergedPr(cwd: string, branch: string): Promise<OpenPrInfo | null> { - // ... duplicate implementation - } + + private async findOpenPr(cwd: string, branch: string): Promise<OpenPrInfo | null> { + return this.findPrByState(cwd, branch, "open"); + } + + private async findMergedPr(cwd: string, branch: string): Promise<OpenPrInfo | null> { + return this.findPrByState(cwd, branch, "merged"); + }
- Replace `openPr`/`mergedPr` with `pr` in server and contracts - Add PR `state` (`open`/`closed`/`merged`) to git status schema - Resolve latest branch PR via `gh pr list --state all` sorted by `updatedAt`
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/server/src/gitManager.ts (2)
216-240:⚠️ Potential issue | 🟠 MajorValidate
GitStatusResultwith the contract schema.
This method returns a shared contract object but skipsgitStatusResultSchema.parse, which can allow malformed payloads to escape. Please validate output for consistency with other endpoints.✅ Proposed fix
-import { - gitRunStackedActionInputSchema, - gitRunStackedActionResultSchema, - gitStatusInputSchema, - type GitStatusPrState, - type GitRunStackedActionInput, - type GitRunStackedActionResult, - type GitStatusInput, - type GitStatusResult, -} from "@t3tools/contracts"; +import { + gitRunStackedActionInputSchema, + gitRunStackedActionResultSchema, + gitStatusInputSchema, + gitStatusResultSchema, + type GitStatusPrState, + type GitRunStackedActionInput, + type GitRunStackedActionResult, + type GitStatusInput, + type GitStatusResult, +} from "@t3tools/contracts"; @@ - return { + return gitStatusResultSchema.parse({ branch: details.branch, hasWorkingTreeChanges: details.hasWorkingTreeChanges, workingTree: details.workingTree, hasUpstream: details.hasUpstream, aheadCount: details.aheadCount, behindCount: details.behindCount, pr, - }; + });As per coding guidelines, "apps/**/*.{ts,tsx}: Use Zod schemas from
packages/contractsfor shared type contracts covering provider events, WebSocket protocol, and model/session types".
437-467:⚠️ Potential issue | 🟠 MajorPrefer open PRs over merged/closed when both exist.
Sorting all states byupdatedAtcan select a merged/closed PR even when an open PR exists for the branch, which would show the wrong status pill. Please prioritize open PRs first, then fall back to the most recent non-open PR. Also guard against invalidupdatedAtvalues for deterministic ordering.🛠️ Proposed fix
- const parsed = parsePullRequestList(parsedJson).toSorted((a, b) => { - const left = a.updatedAt ? Date.parse(a.updatedAt) : 0; - const right = b.updatedAt ? Date.parse(b.updatedAt) : 0; - return right - left; - }); - return parsed[0] ?? null; + const parsed = parsePullRequestList(parsedJson); + const toTimestamp = (value: string | null): number => { + const parsedValue = value ? Date.parse(value) : 0; + return Number.isFinite(parsedValue) ? parsedValue : 0; + }; + const byUpdatedDesc = (a: PullRequestInfo, b: PullRequestInfo) => + toTimestamp(b.updatedAt) - toTimestamp(a.updatedAt); + + const open = parsed.filter((pr) => pr.state === "open").toSorted(byUpdatedDesc); + if (open[0]) return open[0]; + + const nonOpen = parsed.filter((pr) => pr.state !== "open").toSorted(byUpdatedDesc); + return nonOpen[0] ?? null;
Co-authored-by: codex <codex@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/gitManager.ts (1)
219-238:⚠️ Potential issue | 🟠 MajorOpen PR can be masked by a newer closed/merged PR.
findLatestPrselects byupdatedAtacross all states. If a closed/merged PR has a newer timestamp than an existing open PR on the same branch, status will surface the non‑open PR and the UI will disable open‑PR actions incorrectly. Prefer open PRs explicitly before falling back to the latest non‑open entry.🐛 Suggested fix
- let pr: ReturnType<typeof toStatusPr> | null = null; - if (details.branch) { - try { - const existing = await this.findLatestPr(input.cwd, details.branch); - if (existing) { - pr = toStatusPr(existing); - } - } catch { - // PR lookup is best-effort for status rendering. - } - } + let pr: ReturnType<typeof toStatusPr> | null = null; + if (details.branch) { + try { + const open = await this.findOpenPr(input.cwd, details.branch); + const existing = open ?? (await this.findLatestPr(input.cwd, details.branch)); + if (existing) { + pr = toStatusPr(existing); + } + } catch { + // PR lookup is best-effort for status rendering. + } + }Also applies to: 436-466
🧹 Nitpick comments (1)
apps/web/src/components/GitActionsControl.logic.test.ts (1)
28-86: Add coverage for non-open PR states.Now that
pr.statedrives open-PR detection, consider adding a case that asserts"merged"/"closed"behaves like “no open PR” (e.g., create‑PR actions stay enabled). This guards against future regressions in state handling.
Co-authored-by: codex <codex@users.noreply.github.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
|
Bugbot Autofix prepared fixes for 2 of the 2 bugs found in the latest run.
Or push these changes by commenting: Preview (8578146fb9)diff --git a/apps/server/src/gitManager.ts b/apps/server/src/gitManager.ts
--- a/apps/server/src/gitManager.ts
+++ b/apps/server/src/gitManager.ts
@@ -458,6 +458,9 @@
}
const parsed = parsePullRequestList(parsedJson).toSorted((a, b) => {
+ const aOpen = a.state === "open" ? 1 : 0;
+ const bOpen = b.state === "open" ? 1 : 0;
+ if (aOpen !== bOpen) return bOpen - aOpen;
const left = a.updatedAt ? Date.parse(a.updatedAt) : 0;
const right = b.updatedAt ? Date.parse(b.updatedAt) : 0;
return right - left;
diff --git a/apps/web/src/components/Sidebar.tsx b/apps/web/src/components/Sidebar.tsx
--- a/apps/web/src/components/Sidebar.tsx
+++ b/apps/web/src/components/Sidebar.tsx
@@ -1,10 +1,4 @@
-import {
- GitPullRequestIcon,
- MonitorIcon,
- MoonIcon,
- SunIcon,
- TerminalIcon,
-} from "lucide-react";
+import { GitPullRequestIcon, TerminalIcon } from "lucide-react";
import { useCallback, useEffect, useMemo, useState } from "react";
import type { ResolvedKeybindingsConfig } from "@t3tools/contracts";
import { useMutation, useQueries, useQuery, useQueryClient } from "@tanstack/react-query"; |
Co-authored-by: codex <codex@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is ON, but it could not run because the spend limit has been reached. To enable Bugbot Autofix, raise your spend limit in the Cursor dashboard.
| const right = b.updatedAt ? Date.parse(b.updatedAt) : 0; | ||
| return right - left; | ||
| }); | ||
| return parsed[0] ?? null; |
Contributor
There was a problem hiding this comment.
findLatestPr may hide open PR behind stale merged PR
Medium Severity
findLatestPr sorts all PRs (open, closed, merged) solely by updatedAt and returns the first result. If a branch has both an open PR and a merged/closed PR that was updated more recently (e.g., via a comment on the old PR), the merged PR is returned instead of the open one. This causes the UI to show "PR merged" and offer "Create PR" even though an open PR exists, contradicting the stated intent of showing merged state only "when no open PR exists."
Additional Locations (1)
maria-rcks
pushed a commit
to maria-rcks/t3libre
that referenced
this pull request
Sep 21, 2026
i2cjak
referenced
this pull request
in BackplaneWorks/Backplane
Sep 29, 2026
Bots answer people on other machines
Closed
TomCohenDev
pushed a commit
to TomCohenDev/t3code
that referenced
this pull request
Oct 18, 2026
fix(machine-map): moves no longer queue arrows, join labels, steady big red button
TomCohenDev
pushed a commit
to TomCohenDev/t3code
that referenced
this pull request
Oct 18, 2026
An "i" on the frame of the big red button lists what one press does (MM-709 to MM-712), with live counts from the snapshot: the machines that get a stop and the arrows and map roles that drop. While paused it shows who paused and when. It opens as a native popover, and as a bottom sheet on a phone, and never moves the button (pingdotgg#53). Patches MM-708 in apps/machine-map/SPEC.md.
gmackie
pushed a commit
to gmackie/t3code
that referenced
this pull request
Oct 20, 2026
… from T3CODE-33-sync-774c6c1 into custom-local
yasinuslu
added a commit
to yasinuslu/t3code
that referenced
this pull request
Oct 21, 2026
… longer settles the thread' (pingdotgg#53) from fix/settle-only-on-default-branch-merge into dev
kupiainen
added a commit
to upperformance/t3code
that referenced
this pull request
Nov 4, 2026
…e login-era cell — a refused token ends the session's reading, judged for every answer in the one request function and the socket's close under the event permit, without a reconnect loop; the tenant's allowance as the usage limit with its reset time, the runs bound and run bounds as readable failures, and each run's usage as the turn's token usage Re-applies 8e98fbf, 2cc5586 and 823a00d (PR #5 rounds 1-3) on the upstream sync dd2d084, inside its Effect 4.1 / Effect.fn adapter and under upstream's lint rules. A Stop's cancel refines #6's rule: 401 is access loss, any other 4xx a refusal (403 with the cell's reason), every 5xx unconfirmed. One notice-node emission (#6's); the live revoke check requires no socket after the 4401; the allowance check requires the prompt-time usage_limit that vim main sends since vim pingdotgg#53. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kupiainen
added a commit
to upperformance/t3code
that referenced
this pull request
Nov 4, 2026
…ens end the session's reading with what to do, allowance and run bounds reported, run usage as token usage (#5) The fork's cell provider was finished on 2026-10-20 against a cell with no login. This makes it work, and report clearly, against vim `main` (2e7f884 when opened; round 2 rechecked against 4ce3350; round 4 against 25abf41): login and API tokens, 401/403/503, 4401 on a revoked session, suspension, run bounds, the per-tenant daily allowance and runs bound, and per-run usage. The diff stays inside `packages/provider-cell`, its design record, the `live-cell-*` scripts and one cell fixture, so the rebase onto the upstream merge (stream 1) stayed small. **Round 4: rebased onto the upstream sync (fork main dd2d084, #6), see below.** ## What broke against the login-era cell (vim main, login on, before this PR) - A missing, stale or revoked token made the thread fail with "cell provider protocol error: The cell tenant rejected the access token.", with no word on what to do. The probe said the token was rejected even when none was set. - A tenant failing closed (base domain, no `SESSION_SECRET`, 503) showed up as "The cell answered 503 to GET state" in a thread, and as "not a cell tenant" in the probe. - A revoked token (suspension, new invite code) during a run: the socket closes 4401, the reconnect handshake is refused 401, and the adapter reconnected with backoff (up to 10 s) for ever. - A spent daily allowance is refused **at the prompt** (409 `{ runId: null, error }`). The prompt route re-reads the account, so this is the usual path, not the run's `done`. T3 showed it as a provider error ending in "..". - `done.usage` was ignored. - Vim main now retries a model 500 four times and emits each retry as a run-scoped `error`. The adapter published those notices as items without a node, which fails the projection integrity check. The checked-in `cell_wake_after_failure` no longer recorded live: the retries also reorder the child's completion ahead of the run's `done`. Already fine, and verified live: the eight existing scenarios with a token, the mid-run allowance `done` (`usage_limit` + `resetAt`), the runs-bound `done` ("The run did not start: … send the prompt again", thread reusable), and the run-bound `done` texts. Node's fetch and the `ws` client send no `Origin` and no `Sec-Fetch-Site`, so the cell's other-site rule never refuses T3. ## Design - **One error for "the tenant refuses T3 itself"**: `CellAccessError` in `errors.ts`, built from the status and the cell's text. The message names the tenant by host, says the token is missing or was refused and why tokens end, gives the steps (log in at `<base>/login`, Create an API token, Settings → Providers → Cell → Access token), and quotes the cell's reason. The cell's generic "log in at /login, or send … Authorization: Bearer" tail is cut from the quote: the steps say it in T3's terms, and T3's failure redaction would mangle it. The probe (auth `unauthenticated` on 401, `authenticated` when a token's state read succeeds), an attach, a prompt and the socket all use it. Codes: `cell_unauthorized` (401/403), `cell_unavailable` (503). - **Access loss is a terminal state of the session's reader** (reviews 1 and 2 made this hold by construction). It is entered once, by one function (`loseAccess`), under the event permit, and judged in two places only. **The session's one `request` function classifies every answer before its caller sees it**: a 401 to any request (the attach's `GET state`, a prompt, the `GET state` after a lost prompt, a `GET events` page of a replay, a Stop's cancel, a deleted thread's task cancels) enters `loseAccess` right there, whether or not the turn that asked is still in progress, and the caller gets only `CellAccessLost`. If the requesting fiber already holds the permit (a replay, the lost-prompt recovery), the entry happens directly; otherwise it takes the permit. A turn that had already ended gets no second terminal, but the session still reports the error. The other place is **the socket's close handling**: after a 4401 close, or a socket that never opened, its `GET state` answers 401 (handled by `request`) or 403. Entering it discards what is buffered (the turn's candidates, the queued wakes). After it no wake is queued, offered or dispatched (one queueing function, the offer and the dispatch each check the state), nothing already received is read, no request leaves (the request function checks it), the socket is not reopened, and an attach is refused at once. Each turn in progress ends with exactly one terminal, failed with the refusal; every later turn ends at once with it (a start *error* would be shown by the server as its generic "could not start this turn"). A 403/503 refuses one request, not the token: the turn fails with the refusal's message (a Stop: 403 refused with the cell's reason, 503 unconfirmed per #6's rule) and the session reads on. Changing the token rebuilds the instance, so the next message starts clean. - **One admission check on the socket**: after a connection the cell closed with 4401, or one that could not be opened (a refused handshake tells a client nothing), `GET state` is asked once. The event consumer asks it under the permit, after that connection's frames, so a `done` read before the close is applied first. A 401/403 there ends the reading; anything else reconnects with the existing backoff. Ordinary drops cost nothing extra. - **Allowance at the prompt**: a refusal may carry `code: "usage_limit"` and `resetAt`, which the adapter reports as T3's usage limit with the reset time. vim main sends them since vim pingdotgg#53 (25abf41), checked live in round 4; an older cell's words still show, and they name when it renews. - **Usage**: `done.usage` is read leniently (a bad count reports nothing and never makes the `done` unreadable) into `providerTurn.turnTokenUsage`: complete for a completed run, partial otherwise, with `hasSubagents` from the turn's tasks. - **Fake cell**: cell-worded 401s, refused handshakes answered with their status rather than dropped, `revokeToken` (4401 and the stopped run where a run is in progress; idle sockets stay open and deaf, as in the cell), `loginUnset` (503), `usage` on every `done`, `spendAllowance`. The replay transport now records and replays a socket's close code. - **Live tooling**: driver scenarios `access`, `revoke --suspend/--resume <operator token>`, `allowance` and `runs-bound`, each checking its outcome and exiting non-zero otherwise. The probe gains `--token` and 401/503 stand-ins. The scripted API reports usage. - **Docs** (`docs/internals/provider-cell.md`): a new "The login" section (connecting T3 to a deployed cell, what each answer means and what T3 shows, the 30 s policy-cache timing), protocol notes for `usage` and the refusal code, the socket rule, the probe, and a live-check recipe with the login on. ## Review round 1 fixes (2cc5586) Both must-fixes were ways around the refused-token rule. The fix makes the rule hold by construction instead of adding checks at each site. It is the access-loss state described under Design. 1. **A wake buffered during the prompt was dispatched after the prompt's 401.** Before, `loseAccess` cleared the wakes and then released the turn's candidates, which queued and offered the buffered wake again; its dispatch guard did not look at access loss. Now entering the state discards the candidates. The single function that queues a wake, the offer and the dispatch each check the state, so a wake offered *before* the 401 does not dispatch either. Test: `review r1: a prompt refused 401 discards the wakes it buffered and the ones already offered`. 2. **A cancel answered 401 produced no terminal** ("The cell answered 401 to cancel."). Now it enters access loss under the event permit and the turn ends with the refusal as its one terminal. A 403 or 503 on cancel fails the Stop with the actionable `CellAccessError` message and the run goes on. Tests: `review r1: a cancel refused 401 ends the turn with the refusal and the reading`, `a cancel refused 403 or 503 says why and the run goes on`. 3. **Sweep**: `every cell request answered 401 enters access loss once, by one rule` runs every remaining path on a hand-driven socket: `GET state` after a lost prompt, `GET events` during a replay (before the fix this looped reconnecting), 4401 followed by `GET state`, a task's cancel with no turn in progress, and the attach (a retried attach asks nothing). The prompt and Stop paths are covered by the two regression tests above. Each path checks the same things: the session error is entered once, each turn in progress gets one terminal with `cell_unauthorized`, a wake sent afterwards is not read and no continuation is offered, no socket opens past the backoff, no request leaves afterwards, and a later turn fails at once. Negative control: all four tests fail against 8e98fbf (`r2/logs/negative-control.log`: no terminal on cancel 401, a wake offered after the prompt's 401, `ProviderAdapterProtocolError` on a 403 cancel, and the sweep hangs in the `GET events` reconnect loop). ## Review round 2 fix (823a00d) **A prompt left pending, a reset that ended its turn, then the prompt answered 401: no access loss was recorded.** The session then offered and dispatched a wake and sent another prompt. Cause: the prompt's handler returned early on the inactive turn before it looked at the status. Fix: instead of one more check at that call site, the 401 check moved into `request`, which every session request goes through. It runs before any caller logic (turn liveness, candidate release, retries). `tokenRefused`, `loseAccessNow` and the per-caller 401 branches are gone. The permit is now taken through `underPermit`, which records the holding fiber, so a request made under the permit enters access loss without taking the permit a second time. A task cancel's shorter bound is now a `timeoutMs` on the request, so the bound can never cut off the entry into access loss. The socket's post-close `GET state` now goes through the event consumer (`check_access` inbox entry), so it is ordered behind the connection's frames. Test: `a 401 to a request whose turn had already ended still enters access loss, with no second terminal`. It covers the reviewer's ordering (prompt pending → reset ends the turn → 401) and the same ordering for a Stop's cancel and for a `GET events` page (the page before it carries the reset). It also checks that a run's `done` followed by a 4401 close completes the turn before access is lost. Each case checks one session error, no extra terminal, no wake offered or dispatched, no request or socket afterwards, and a later turn failing at once. The driven cell gained `parksCancels()`. Negative control: against 2cc5586's adapter the test fails, timing out on the prompt ordering (no session error ever comes) (`r3/logs/negative-control.log`). ## Round 4: rebase onto the upstream sync (head 8e4f01e, base dd2d084) **How:** re-applied as one new commit on dd2d084, not a commit-by-commit rebase. #6 rewrote the adapter: Effect 4.1, `Effect.fn` conversions, upstream's comment cuts, and the new Stop path. Each of my three commits touched the same Stop and task-cancel regions, so a sequential rebase would have resolved them three times. I did a 3-way merge of the squashed change (cfa469e → 823a00d) against dd2d084: 17 conflict hunks in 9 files. The old commits stay reachable as 8e98fbf / 2cc5586 / 823a00d. The commit message names them. **Reconciled with #6:** - **Stop rule, refined:** a 401 to a Stop's cancel is access loss. `request` enters it, the turn ends with the refusal as its one terminal, and Stop returns. Any other 4xx is a refusal (`ProviderAdapterInterruptError`; a 403 carries the cell's reason as `CellAccessError`), and 409 keeps its meaning. Every 5xx (503 included, which round 3 treated as a refusal), a transport failure or no answer is `ProviderAdapterInterruptTimeoutError`. #6's 5 s cancel + 8 s confirmation bounds are kept. The cancel bound is passed as `request`'s `timeoutMs`, so it never cuts off the entry into access loss. The test `a cancel refused 403 says why, a 503 leaves it unconfirmed, and the run goes on` replaces round 3's 403/503 test. - **Task cancels on deletion:** #6's `Effect.fn` form plus the access-loss guards and `timeoutMs` inside `request`. - **Notice node:** one emission (#6's). My live notice test duplicated #6's driven-socket test, so I dropped it and folded its one extra assertion (the node is a `system` node) into #6's test. - **Close codes:** the `closeCode` mapping moved into #6's NodeWS socket reader. The replay transport still records `ws_closed` with its code. - **`cell_wake_after_failure`:** I kept #6's recording and #6's recipe (`MODEL_RETRY_DELAY_MS=1` in `.dev.vars`, no `CHILD_DELAY_MS`), and dropped my `CHILD_DELAY_MS=15000` text from the docs. I re-recorded it against a fresh vim 25abf41 cell with the login off. It differs from the checked-in transcript only by ids and timestamps (4 retry notices, 22 entries, replay-verified), so this PR no longer changes the fixture. No new fixture, so nothing new to register in CellReplayFixtures. - **Upstream lint:** I cut 11 long comments to ≤ 4 lines (the design stays in docs), used `catchTags`, and un-exported `cellRefusalText` (knip). The four real-clock waits are the absence-of-reconnect checks after access loss. They keep `// oxlint-disable-next-line t3code/no-real-time-wait-in-tests` with main's existing justification ("an absence has no event to await"). There's no cheap event: making them event-driven would need a production hook that exposes the socket loop's end. The fourth is inside the `assertAccessLost` helper, which the rule does not flag. - **`requiresSignIn`: not adopted.** It is a flag on `ProviderAdapterOpenSessionError`, and the cell's openSession makes no request. The tenant is first asked at the thread attach, which needs the project. The server's only use of the flag is to skip the start worker's retries (100+200+400+800 ms). Those retries already send nothing, because the refusal is cached in the session. Adopting it would add a `GET state` to every session open, and it could not cover a 401 that arrives mid-session. - **Round-3 should-fix:** the live `revoke` check now requires `socketsAfterClose === 0`. **`allowance`** now requires the prompt-time `usage_limit` with `resetAt`, which vim main sends since pingdotgg#53. ## Verification Round 4 (head 8e4f01e, on dd2d084; logs `/srv/t3/tmp/ready-2026-11-04/t3code-login-era/r4/logs/`): - Root: `vp run -r --concurrency-limit 4 typecheck` **41/41**. `vp lint --report-unused-disable-directives` exits 0 with main's four warnings and none in touched files. `vp fmt --check` is clean. `pnpm run knip:check` is clean. The pre-commit hook (fmt, lint, knip) passed. - `packages/provider-cell`: `pnpm test` **113 passed** (3 files; #6 main has 97). `pnpm typecheck` (tsc) is clean. - `apps/server`, `CLAUDE_CONFIG_DIR` unset, by path: CellRestartAdoption, CellThreadDeletion, **CellReplayFixtures**, RunExecutionService, ProviderSessionManager, ThreadDeletion, ProviderRuntimeRecoveryService, ProviderTurnStartService, ProjectService.deletion, Orchestrator.control-reads, ManagedPolicy, ProviderTurnControlService, RestartBackgroundNote and ProviderSwitch.integration: **14 files, 274 passed**. - The review r2/r3 scratch orderings (11 tests: late prompt 401 after a reset, late 202 after socket loss, socket/prompt and cancel/done races, concurrent task cancels past their bound under a held permit, closing with an interrupted requester, prompt 403/503, 4401 then 403 vs 503) were inserted temporarily into adapter.test.ts: **13 passed** with the two own `review r1` tests (`review-orderings.log`). - **Live, vim main 25abf41** (built fresh, index.js sha256 3b7e9109…), celld 0.6.2, scripted API, `MODEL_RETRY_DELAY_MS=1`, `bash r4/live/run-live.sh r4`: **35/35 exit 0**. - Login on: probe; `shell`, `interrupt`, `access` (no token, bogus token, fail-closed tenant); `revoke` (4401 at +30 s, **0 sockets after it**, refusal names the suspension, next message fails at once, resume posted); `shell` after re-login; `delete-delegate` (cancel 200, listed cancelled); 6 recordings; `allowance` at the prompt (**409 carries `code: usage_limit`, turn fails as `usage_limit`, `resetAt` 2026-11-05T00:00Z**) and mid-run; `runs-bound`; fail-closed probe. - Login off: probe; `shell`, `interrupt`, `limit`, `delegate`, `reconnect`, `restart`, `attach-retry`, `delete-delegate`; all **7** recordings, `turn_interrupt` and `cell_wake_after_failure` included. - Live usage on the turn: input 4088 / output 59. Round 3 (head 823a00d): - `packages/provider-cell`: `pnpm test` **112 passed** (111 before, +1 sweep test with four orderings). `pnpm typecheck` clean. `vp lint --report-unused-disable-directives` and `vp fmt --check` clean. - `apps/server` cell suites by path: **10 passed** (143 skipped). - **Live against vim main 4ce3350** (the round-2 build; vim main is now 7888f18, but cell code is unchanged since 4ce3350), login on, full `run-live.sh`. Passed: the probe, `shell`/`interrupt`/`limit`/`delegate`/`reconnect`/`restart`/`attach-retry`, `access` (no token, bogus token, fail-closed tenant), `revoke` (refusal names the suspension, next message fails at once, **0 sockets after the 4401**), `shell` after re-login, `delete-delegate` (the cell lists the task as cancelled), allowance at the prompt and mid-run, and `runs-bound`. 24 of 25 exit 0; `turn_interrupt` with the login on fails as before (owner question 3). Round 2 (head 2cc5586): - `packages/provider-cell`: `pnpm test` **111 passed** (107 before, +4). `pnpm typecheck` clean. `vp lint` and `vp fmt --check` clean. - `apps/server` cell suites by path (`CellRestartAdoption`, `CellThreadDeletion`, `OrchestratorReplayFixtures -t cell`): **10 passed**. - **Live against vim main 4ce3350** (built fresh, celld 0.6.2, login on, tenant `default`, API token via `/login` + `/login/token`): probe; `shell`, `interrupt`, `limit`, `delegate`, `reconnect`, `restart`, `attach-retry` with the token; `access` with no token, a bogus one, and against a fail-closed tenant; `revoke` (refusal names the suspension, next message fails at once, **0 sockets after the 4401**); `shell` after re-login; `delete-delegate`; `allowance` at the prompt and mid-run; `runs-bound`. All exit 0. Six of seven recordings pass; `turn_interrupt` with the login on fails as in round 1 (owner question 3). Round 1 (head 8e98fbf, vim main 2e7f884): - `packages/provider-cell`: `pnpm test` went from **95 to 107 passed** (3 files): +2 protocol, +1 scripted socket (4401, then `GET state` 401, nothing after), +9 over the fake cell (probe answers, refused attach, revoke mid-run with no reconnect, prompt 401 on a deaf socket, refused reconnect, allowance refusal, runs bound, notice node, usage). `pnpm typecheck` is clean. - `apps/server`: the cell suites by path (`CellRestartAdoption`, `CellThreadDeletion`, `OrchestratorReplayFixtures -t cell`): **10 passed** before and after, with the refreshed fixture. `pnpm typecheck` shows no diagnostics in `scripts/live-cell-*` or provider-cell. - `vp lint --report-unused-disable-directives` and `vp fmt --check` on the changed files: clean. - **Live, vim main 2e7f884 on celld 0.6.2, login on** (`CELL_BASE_DOMAIN=vim.test`, `SESSION_SECRET`, `INVITE_SECRET`, tenant `default` at the apex, API token minted through `/login` + `/login/token`), full run `run-live.sh r3`. Passed: - the probe; - `shell`, `interrupt`, `limit`, `delegate`, `reconnect`, `restart` and `attach-retry` with the token; - `access` with no token, with a bogus one, and against a fail-closed tenant (no prompt posted in each); - `revoke`: 4401 at +30 s, **0 sockets after it**, the second message ended at once with the refusal, resume posted; - `shell` after re-login, `delete-delegate`; - `allowance` at the prompt (`TENANT_DAILY_TOKENS=1000`) and mid-run (`41500`, `usage_limit` + `resetAt`); - `runs-bound` (`TENANT_MAX_RUNS=1`); - six of seven fixture recordings with the token; - live usage on the turn (`inputTokens 4060, outputTokens 59`). - `cell_wake_after_failure` was re-recorded against that cell (`CHILD_DELAY_MS=15000`, `--write-fixture --version 0.3.0`) and checked in. The transcript carries no token. - `turn_interrupt` records with the login **off** (5/5) but not on (0/5); see owner questions. ## Owner questions (provisional decisions) 1. ~~The allowance refusal's code~~: settled by vim pingdotgg#53 (the prompt's 409 carries `code: "usage_limit"` and `resetAt`), mapped live in round 4. 2. **A refused token ends the session's reading** rather than retrying it. I chose this because a refused token never holds again. Is a session-level stop right, or should later turns retry the attach (costing one more 401 each)? 3. **`turn_interrupt` with the login on**: the shared input presses Stop right after the prompt, which with the login on lands during the first model request's reservation, so the shell row never appears. Adding `waitForTurnItemType: "command_execution"` to the shared input would fix it, but that touches every provider's fixture and `fixtures/index.ts`; I left it. 4. Failure codes `cell_unauthorized` / `cell_unavailable` are new strings. The class stays `provider_error`, since contracts have no auth class. Co-Authored-By: Claude Opus 5.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
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.
Summary
GitStatusResultwith a nullablemergedPrfield in shared contracts.gh pr list --state merged) when no open PR is found.openPr/mergedPrasnull.Mergedstatus pill when branch-matched and no open PR exists.mergedPrbehavior.Testing
apps/server/src/gitManager.test.ts: verified merged PR fallback lookup, no merged lookup when open PR exists, and failure resilience (openPr/mergedPrnull).apps/server/src/wsServer.test.ts: updated status expectations to includemergedPr: null.apps/web/src/components/GitActionsControl.logic.test.ts: updated status fixture to includemergedPr: null.packages/contracts/src/git.test.ts: updated schema parsing test to includemergedPr: null.Summary by CodeRabbit
New Features
Bug Fixes
Note
Medium Risk
Changes the shared
GitStatusResultcontract and server status flow to callgh pr list --state alland return the latest PR with a derived state; incorrect parsing/sorting orghbehavior could cause misleading PR status or extra load.Overview
Git status now returns a unified
probject (ornull) withstate(open/closed/merged), replacing the previousopenPrfield in the shared contracts and all callers.On the server,
GitManager.statusnow queries GitHub viagh pr list --state alland picks the most recently updated PR, normalizingMERGED/mergedAtintostate: "merged"while keeping best-effort behavior (failures yieldpr: null).On the web, git actions/CTAs now treat “open PR” specifically as
gitStatus.pr?.state === "open", and the thread sidebar periodically queries git status per thread/worktree to display a PR-state icon (open/closed/merged) when the thread’s branch matches the repo status.Written by Cursor Bugbot for commit 4b3825e. This will update automatically on new commits. Configure here.
Note
Display merged PR status in the thread sidebar by returning
prwithstatefromGitManager.statusand removingopenPrRename
openPrtoprwith astatefield in the Git status contract, add latest-PR lookup across all states inGitManager.status, and render a per-thread PR state icon in the sidebar. Update server and web logic/tests to usegitStatus.pr?.state === "open"for actions and UI.📍Where to Start
Start with the status flow in
makeGitManager.statusin apps/server/src/git/Layers/GitManager.ts, then reviewfindLatestPr,parsePullRequestList, andtoStatusPr, followed by the sidebar integration in apps/web/src/components/Sidebar.tsx.Macroscope summarized e8dfc42.