Uh oh!
There was an error while loading. Please reload this page.
fix(cli): discover resumable sessions from /resume - #3582
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at exact head e23405ce34db55480faf0f2a269cc2105935c8a2.
Coverage: how /resume discovers sessions, and the call chain from picking one to actually resuming. Not covered: wording.
Thanks for adding this entry point — the gap it targets is real. Two things need fixing before it does what it says, and both are reproducible on the ordinary user path.
[P2] onlyResumable filters on "attachable", not "has a safe boundary to resume"
packages/cli/src/pi-tui-runner.ts:2214-2215 selects entries with availability.get(session.id)?.available === true. But that availability comes from inspectSessionResumeAvailability at packages/cli/src/session-driver.ts:182-190, which only checks that session.cwd exists and is realpath-able. The Runtime Host implementation is the same shape, and for remote sessions it returns true for anything that has a cwd at all.
The authority for "is there something to resume" is elsewhere: the Host's turn.resume.query, where the Runtime plans a continuation only from that session's failed/cancelled inline run and otherwise reports resume_candidate_missing. The list path never calls it, and does not consult runningTurnIds or corrupted/unexecutable state either.
So an ordinary completed session, a session that is currently running, or one with no continuable run all appear in /resume as long as their working directory still exists. On remote, runtime-host-tui-command.ts:97-102 pins the scope to all while the new path applies no project predicate, so sessions from other projects are listed too. The same weak predicate also drives the startup hint at pi-tui-runner.ts:2282-2297, which will tell the user a plain attachable session "has an interrupted run".
Suggested direction: have a Runtime-owned resumability query return each session's real disposition, and filter here on current project, deleted workspace, running, and corrupted state — rather than reusing SessionResumeAvailability, which only answers cwd attachability.
[P2] Selecting from the picker does not actually resume
With no attached session, pi-tui-runner.ts:2134-2137 opens showSessionList({ onlyResumable: true }) and returns. The picker's onSelect at :2251-2260 calls goToSession(item.value), which on the idle path runs switchSession (:1649-1652) — and the comment right above goToSession says it plainly: /session is view navigation.
switchSession in runtime-host-session-driver.ts:471-524 validates cwd and the execution boundary, opens a subscription, and attaches a still-running root turn. It never calls turn.resume.query or turn.resume.start. The only driver path that starts a safe-boundary continuation is resumeLatest() at :325-349.
The user therefore picks a session, the picker closes as if the action succeeded, and the interrupted turn does not continue — they typically have to type /resume a second time so the current-session path reaches resumeLatest. That is the core function of the new entry point.
The new test at pi-tui-runner.test.ts:3265-3292 only asserts list text and filtering; it does not press Enter and assert that resumeLatest or a turn start follows, which is why this got through.
Suggested direction: make the selection path call a Runtime-owned switch-and-resume operation, or call resumeLatest once explicitly after a successful switchSession, keeping race/parked/failure outcomes visible.
Note on CI
check-runs on this head is total_count 0 — no run at all, which is neither green nor red. Even once the two items above are fixed, the gate needs terminal green on the new head.
Verification and limits
Changed-file Biome and git diff --check pass. A full CLI build could not be completed in our environment — pre-existing workspace drift unrelated to this PR blocked it — so no targeted-suite pass is claimed here; the findings above are established by reading the call chains at this exact head.
mikemikimike
commented
Aug 23, 2026
Follow-up fix pushed in the new head.
Verification on the new head: CLI build, typecheck, lint, format check, |
acc96c0 to
f2c24a3CompareAstro-Han
left a comment
There was a problem hiding this comment.
Reviewed at exact head f2c24a35. Two [P1]s, both inline. Publishing on behalf of a reviewer without write access here; the analysis is theirs, and I re-verified both against this head before posting.
Gate status is red, but not because of this PR. The test job's Build step fails on apps/desktop/src/main/__tests__/goal-services-adapter.test.ts:64,70,76 with TS2353: 'type' does not exist in type 'SessionChangedEvent'. That file is untouched here — this PR changes four packages/cli files. The root cause was already fixed on main by cded195 (#3642), so merging current main into this branch and re-running should clear it.
The direction is right: /resume should be able to open a picker when there is no current session, and startup should tell you an interrupted run exists. The two findings are both about the resume predicate being reused where an attachability predicate is what the caller needs.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at 253fb50e595de0604c0c22b303fae1fa8023af82. Both of the [P1]s we raised earlier are fixed by this commit. No P0-P2 remain. Two gate blockers below, one of which is not yours to fix in code.
Correcting our own scope first
An earlier round of this review was working from a 252-file, +7405/-6516 diff. That is not what this PR changes. This branch was rebased onto main and replayed roughly twenty squash-merged commits; because squashing produces new SHAs, git cannot see them as shared history, so the merge-base is stuck at an old commit and every merge-base-relative view — the Files tab and the REST file list included — inflates accordingly. Those views are not wrong; they accurately report merge-base-to-head, which is simply not the same question as "what did the author change here".
The author-owned delta is one commit:
253fb50e fix(cli): separate session attach and resume discovery
packages/cli/src/__tests__/pi-tui-runner.test.ts +30
packages/cli/src/pi-tui-runner.ts +37/-14
packages/cli/src/runtime-host-session-driver.ts +17/-17
packages/cli/src/session-driver.ts +1
4 files, +68 / -17
We are stating this because we got it wrong in this thread before stating it, and because the wrong number changed a conclusion further down.
Both prior [P1]s are closed
Attach availability no longer answers the resume-candidate question.getSessionResumeAvailability drops its host branch and returns inspectRuntimeHostSessionResumeAvailability unconditionally; the narrow turn.resume.query path moves into a new getSessionResumeCandidateAvailability. showSessionList picks by mode, and — this is the line that fixes the reported symptom — the selection handler changed from if (availability.get(item.value)?.available === false) return; to if (options.onlyResumable && availability.get(item.value)?.available === false) return;. /session no longer refuses to navigate to a session because a resume-candidate check said false. The commit subject says exactly this, and the code matches it.
The startup enumeration race is fixed at the mechanism, not by moving the timer. A memoised sessionListPromise now backs a shared listSessions(), so showSessionList and announceResumeAvailability join one in-flight enumeration instead of issuing two. announceResumeAvailability also returns early when the driver lacks getSessionResumeCandidateAvailability. The setTimeout(..., 0) is still there, and that is fine: the timer was never the defect — the concurrent duplicate enumeration was, and it is now memoised away.
One supporting argument from our earlier review should be retired rather than repeated: we noted that the availability map was built with a bare Promise.all(sessions.map(...)) while the sibling foreign-session branch had per-item containment. That asymmetry is gone — and specifically, this commit is what added the per-session try/catch. It is not that our evidence went stale; the author fixed it.
Gate: two blockers, both needing one push
1. Formatting. The test job fails at Check formatting on pi-tui-runner.ts around the new resume-availability expression. That step runs before the tests, which has a consequence worth naming: no test on this head has executed, so CI currently cannot tell you anything about the change itself. npm run format resolves it.
2. TS2304: Cannot find name 'decodeStoredMessage' in the package job — please do not hand-edit this. The symbol is in packages/storage/src/__tests__/codex-session-adapter.test.ts:267, which your commit does not touch. The branch replayed the older versions of #3562 and #3520 in the order that briefly broke main: the rename landed first, then an assertion still using the old symbol. main was repaired afterwards by #3656, but this branch replayed before that fix existed. Rebasing onto current main brings #3656 with it and clears this. Editing line 267 by hand would mean changing a file that is not part of your change.
The PR is also CONFLICTING, so a rebase is required regardless — one push can settle all three.
One thing to check on your side
The two review threads above show replies from you reading System.Collections.Hashtable[...]. That is a PowerShell object stringified by whatever tooling posted them, so no text of yours actually arrived. From the GitHub UI the threads look answered, which is why we are pointing it out rather than assuming you chose not to reply. If you meant to say something there, it will need reposting.
Astro-Han
left a comment
There was a problem hiding this comment.
Correction to my previous review on this PR (5004256305), specifically to the scope paragraph.
I wrote that the author-owned delta is one commit, 4 files, +68/-17. That is wrong. It is three commits and 5 files.
9e24a005 fix(cli): discover resumable sessions from /resume 2 files, +63/-2
f2c24a35 fix(cli): resume the selected interrupted session 4 files, +25/-6
253fb50e fix(cli): separate session attach and resume discovery 4 files, +68/-17
Union: pi-tui-runner.ts, runtime-host-session-driver.ts, session-driver.ts, __tests__/pi-tui-runner.test.ts, __tests__/runtime-host-session-driver.test.ts. The +68/-17 I quoted is the tip commit alone, not this branch's contribution.
How I got it wrong is worth stating, because the previous review's whole first section was about scope. I listed the commits between the merge-base and the head, truncated the output, and read the top entry as the PR's own work. The other two author commits are further down that list, interleaved among the twenty-odd replayed main commits. A truncated observation reported as a fact — the same failure mode I was describing one paragraph earlier.
The reliable way to separate them is patch equivalence rather than reading a commit list:
git log --cherry-pick --right-only --no-merges origin/main...<head>
which returns exactly these three and marks the other nineteen as already present on main. Credit to a reviewer on our side for pointing out that subject-line matching is not sufficient here; this is what caught my error.
Nothing else in that review changes. Both [P1]s are still closed, and I verified that by reading the tip commit's actual diff — the removal of the host branch from getSessionResumeAvailability, the new getSessionResumeCandidateAvailability, the options.onlyResumable && guard on the selection handler, and the memoised sessionListPromise. The two gate blockers are unchanged: run npm run format, and rebase onto current main so that #3656 clears the TS2304 on a file that is not part of your change.
The practical difference for you is only in how much of this branch a reviewer should be reading: five files across three commits, not four files in one.
Astro-Han
left a comment
There was a problem hiding this comment.
Correcting one sentence in 5004276704 — a tooling claim, not a conclusion about this PR.
I wrote that
git log --cherry-pick --right-only --no-merges origin/main...<head>
"returns exactly these three and marks the other nineteen as already present on main". It does not mark anything.--cherry-pickomits commits whose patch is already present on the other side; nothing in its output refers to them. The marking form is git cherry -v (+ / -) or --cherry-mark, which prints = for a patch-equivalent commit and > for one only on the right side. On this branch --cherry-mark gives 19 = and 3 >.
Two limits on that command that my wording also glossed over:
It produces a candidate classification, not a proof of ownership. A replayed commit rewritten during conflict resolution is no longer patch-equivalent and will appear as author work; conversely, a commit the author genuinely wrote that happens to be patch-identical to something already on main is omitted and silently disappears from their delta. The first direction costs extra reading, the second can hide real author work, and neither is detectable from the command's output alone.
--no-merges is --max-parents=1, so any delta introduced by a merge commit's own conflict resolution is excluded. That is harmless here — this branch has no merge commits, it was rebased — but the exclusion is silent, so it is worth stating rather than assuming.
None of this changes the facts in the previous correction, which an independent reviewer on our side re-derived from a fresh fetch: three author commits (9e24a005, f2c24a35, 253fb50e), five files in the union, none of them the Codex adapter test, and all three on this head's first-parent chain. The [P1] closures and the two gate items are likewise unaffected — still npm run format, and rebase onto current main so #3656 clears the TS2304.
Astro-Han
commented
Aug 24, 2026
CI note, with one part that needs your judgement rather than a mechanical fix. This head's checks had never executed — the run was parked awaiting maintainer approval. I released it ( Unlike the other parked runs I released today, this one gets all the way through. Lint, formatting, the Astryx inventory, 1.
The id column is eight characters wide, so 2. What I established:
The expected/actual gap is not a near-miss. It expects the session list to render a row reading roughly and then an otherwise empty screen — no list at all, and a resume that did happen. The test's whole point is that this session must stay visible but must not be resumable. I am not going to guess which of your changes causes it, because two new paths could and I would rather you check than have me assert. For what it is worth, both are in
To be fair to the second one: Neither failure is flaky. The first is deterministic by construction, and the second contradicts a green run on its own merge base. Both are reproducible with 中文CI 提示，其中一条需要你判断，不是机械修复。 这个 head 的检查此前从没执行过——run 卡在等 maintainer 放行，我已放行（ 与我今天放行的其他几个不同，这个 run 一路走到了很后面：lint、格式、Astryx 清单、 1.
2. 已确立的事实：这个测试在 merge base 期望与实际的差距不是"差一点"：它期望会话列表渲染出一行大致为 我不去猜是哪一处改动导致的，因为有两条新路径都可能，我宁愿你去查，也不愿我来断言。两条都在 为第二条说句公道话： 两个失败都不是 flake：第一个由构造决定必然失败，第二个与它自己 merge base 上的绿色 run 相矛盾。都可以用 |
317e737 to
fe3d012Comparefe3d012 to
6127b10CompareGenerated-by: Codex
Generated-by: Codex
Generated-by: Codex
6127b10 to
a4cab4eComparemikemikimike
commented
Aug 27, 2026
Thanks for the CI report. I rechecked the exact current head 4e85213. The truncation assertion is now on the visible id form (attachab), and the regression test now also selects an attachable row whose resume availability is false and observes switchSession. The loading test exercises the startup advisory capability and asserts a single listSessions call. Both focused tests pass, including five repeated combined runs. The CLI build, typecheck, changed-file Biome check, and git diff --check pass. The GitHub check for this new head is currently queued; no terminal CI result is available yet. |
me2seeks
left a comment
There was a problem hiding this comment.
Reviewed at exact head ed945f569af75199991994c679ba3a672aa2fb12. The prior attach/resume separation and no-cwd fixes are present, and CI is green. One ordinary local-TUI path still violates the resumable-only contract; details inline.
| ? projectedSessions.filter(({ session }) => session.cwd === cwd) | ||
| : projectedSessions; | ||
| const items: SelectItem[] = visibleSessions.map(({ session, depth }) => { | ||
| const selectableSessions = options.onlyResumable |
There was a problem hiding this comment.
[P1] /resume still offers foreign import sessions that are not safe-boundary candidates.
This onlyResumable filter applies only to the projected Maka sessions. The same showSessionList call still runs foreignSessions.listSessions({ cwd }), and the render path later appends every foreignByValue row unconditionally. The production local TUI always supplies foreignSessions, so with no attached Maka Session, /resume can show a Codex/Claude row. Selecting it takes the importForeignSession branch, creates a new Maka Session, and starts a handoff turn; it never passes turn.resume.query and is not a safe-boundary continuation. That contradicts the PR summary that this picker is limited to sessions whose resume availability is ready, and can turn a recovery command into an unrelated provider call.
Please skip the foreign scan and foreign rows when options.onlyResumable is true, while preserving them for /session. A regression should use no attached Maka Session plus one foreign session, invoke /resume, and prove that the foreign row is absent and neither import nor a new turn occurs.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the focused follow-up—the earlier attach/resume and foreign-session issues appear materially addressed. I found two bounded resume-path edges worth tightening. This is an AI-assisted review; I independently traced the current TUI and Host catalog paths. These are suggestions from an outside perspective, so please do push back if I missed a lifecycle or catalog invariant.
| void goToSession(item.value); | ||
| void (async () => { | ||
| await goToSession(item.value); | ||
| if (options.onlyResumable) await runControl(resumeSession); |
There was a problem hiding this comment.
Thanks for fixing the no-attached /resume flow. In a category ② stale-selection path, could this only call resumeSession after a successful switch? goToSession() wraps switchSession() in runControl(), and runControl() reports errors but resolves normally. If the selected Session is archived/deleted or becomes ineligible while the picker is open, the failed switch therefore falls through here and reopens the same /resume picker; an ineligible-but-still-listed row can repeat that loop. I suggest returning an explicit success result (or combining switch + resume into one control action) and covering a stale selection. This looks P2 because Esc recovers, but please push back if another guard makes the selected row stable.
| @@ -2457,18 +2471,31 @@ export async function runMakaPiTui(input: MakaPiTuiInput): Promise<void> { | |||
| const [availabilityEntries, foreignScan] = await Promise.all([ | |||
| Promise.all( | |||
There was a problem hiding this comment.
Thanks for bounding the foreign import side of this picker. The Maka-session side still appears unbounded on the normal /resume path: listSessions() materializes the complete paged Host catalog, then this Promise.all sends a turn.resume.query for every historical Session before the UI applies the current-cwd filter below. The 32-row protocol bound is only per page, so catalog size directly becomes memory, RPC fan-out, and open/startup latency (the startup hint uses the same discovery). Could the Host/catalog query apply cwd/age/cursor/limit at the source boundary, or at least filter first and bound query concurrency, with a large-history regression? I would rate this P2 as a local scale/performance issue; please push back if the catalog has a documented total-size bound I missed.
Summary
Fixes#3508.
When the TUI has no attached session,
/resumenow opens a picker limited to sessions whose resume availability is ready, instead of attempting an attached-session-only resume. Startup also performs a best-effort availability check and surfaces a passive hint for the current or cwd session when safe-boundary resume is available.The follow-up in this PR restores the guard for sessions without a working directory while keeping cwd-backed
/sessionrows attachable when resume discovery is unavailable for another reason.Implementation
/resumewithout an attached session./sessionrows, attached-session resume behavior, sandbox boundaries, and Desktop policy unchanged.Verification
npm --workspace maka-agent run build— passed.node --test --test-name-pattern="keeps live status visible for a session without a cwd but prevents resuming it|/resume opens a picker containing only resumable sessions when none is attached|/session keeps attachable rows when resume discovery fails for another session" packages/cli/dist/__tests__/pi-tui-runner.test.js— 3 passed.node --test packages/cli/dist/__tests__/runtime-host-session-driver.test.js— 59 passed.npx --no-install biome format packages/cli/src/pi-tui-runner.ts packages/cli/src/runtime-host-session-driver.ts packages/cli/src/session-driver.ts packages/cli/src/__tests__/pi-tui-runner.test.ts packages/cli/src/__tests__/runtime-host-session-driver.test.ts— passed.npx --no-install biome lint packages/cli/src/pi-tui-runner.ts packages/cli/src/runtime-host-session-driver.ts packages/cli/src/session-driver.ts packages/cli/src/__tests__/pi-tui-runner.test.ts packages/cli/src/__tests__/runtime-host-session-driver.test.ts— passed.npm --workspace maka-agent run typecheck— passed.git diff --check— passed.pi-tui-runnertest file — 142 passed, 2 failed in unrelated Windows SIGTERM restoration tests; Linux CI is authoritative for the supported workflow.Not run: the full repository build, package-level
npm --workspace maka-agent test, and Docker-based checks; this is a pure CLI/TUI unit behavior change with no external service dependency.AI use
Tool(s) and scope: Codex assisted with remote issue selection, implementation, regression testing, and PR text. The commit includes a
Generated-by: Codextrailer.Checklist
Does this PR entail a change in behavior?
/resumenow discovers resumable sessions when no session is attached, while/sessionkeeps attachable rows available.