Repository navigation
fix(cli): discover resumable sessions from /resume - #3582
mikemikimike wants to merge 36 commits into
Conversation
Astro-Han
left a comment
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.
|
Follow-up fix pushed in the new head.
Verification on the new head: CLI build, typecheck, lint, format check, |
acc96c0 to
f2c24a3
Compare
Astro-Han
left a comment
Astro-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.
Astro-Han
left a comment
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
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
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-pick omits 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.
|
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
fe3d012
Compare
fe3d012 to
6127b10
Compare
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
6127b10 to
a4cab4e
Compare
|
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
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
Astro-Han
left a comment
There was a problem hiding this comment.
Third independent review lineage at 5830c27e (supplements the two earlier reviews on this head).
The bounded paging, duplicate-cursor guard, RevisionChanged retry, concurrency cap and stale-selection handling all look correct. I found one additional P2, the flip side of the limit-truncation issue already raised:
The page cap discards everything already found (inline). Pages hold 32 items (SESSION_CATALOG_PAGE_MAX_ITEMS), so 8 pages cover only the newest 256 catalog rows, archived sessions and side conversations included. In current-workspace mode, the driver needs 200 same-cwd sessions within those rows. If it can't find that many and a cursor remains, it throws MakaSessionCatalogIncompleteError and drops sessions. Example: a user with ~10 projects × ~30 sessions runs /resume and gets only the "discovery stopped" notice. The picker never opens, even though the interrupted session was on page 1. The startup "/resume to continue" hint takes the same path and is swallowed by catch {}. So the feature fails most reliably for the heaviest users. Suggest returning partial results with an incomplete flag and showing the notice alongside the picker.
Non-blocking P3s:
- Unrelated scope. The external-import action picker was restructured (including removing the inner
busy || turnRunningguard, as noted earlier), and the existing testopens the latest imported task without importing another copywas deleted without explanation. That test covers exactly this picker, so please restore it or split the import changes out. - Weaker retry. The bounded reader re-implements stable-read retry with a fixed 3 attempts and no backoff, which is weaker than the previous helper.
- Startup hint copy doesn't fit the no-mounted-session case.
Not verified: tests were not run locally (static read).
This review was produced with automated assistance (AI review agents) and checked by a maintainer-side reviewer before posting.
| } | ||
| if (cursor !== undefined && pagesRead === MAX_SESSION_CATALOG_SCAN_PAGES) { | ||
| throw new MakaSessionCatalogIncompleteError(pagesRead); | ||
| } |
There was a problem hiding this comment.
P2 — Hitting the page cap throws and discards the sessions collected so far. With 32-item pages, 8 pages cover the newest 256 catalog rows. A current-workspace scan that finds fewer than limit matches there (common for users with many projects) fails outright. /resume then shows only the incomplete notice, even when the target session was on page 1. Returning { sessions, incomplete: true } and letting the picker show partial results plus the notice would keep the feature usable exactly where it's most needed.
Generated-by: Codex
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
The latest commit prevents an already-imported external source from being imported again while a turn or control action is running. I did not find a new issue in that guard. The resume picker still has a correctness gap: it stops after 200 catalog candidates before checking which ones are actually resumable, so a ready session beyond that prefix can be hidden without the incomplete-discovery notice. I left the actionable finding inline.
Current-head CI test passes. Locally, Node 24 build:test, 313 relevant CLI tests, and the TUI copy check pass; a production-driver probe returned 200 unavailable candidates while the omitted 201st was ready. The PR diff-check and static merge against current main are clean. I did not run a packaged TUI or a real remote Host with a large catalog.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| } | ||
| if (limit !== undefined && sessions.length >= limit) break; | ||
| } | ||
| if (!page.nextCursor || (limit !== undefined && sessions.length >= limit)) { |
There was a problem hiding this comment.
[P2] Do not mark this catalog read complete merely because 200 raw Session candidates were collected. The /resume picker requests { limit: 200, cwd }, then checks resumability afterward (pi-tui-runner.ts:3401,3423-3450). If the first 200 matching Sessions cannot resume but the 201st can, this branch clears nextCursor, returns a complete-looking list, and the picker shows no match without the incomplete notice. I reproduced that on this head with the production driver: 200 listed, 0 ready, while a direct check of omitted session-200 was ready. Keep a partial signal or continue catalog discovery based on ready candidates, and add a regression with this distribution.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Independent re-review of head 2461c100 (Claude lineage). Compared with the previous head 5830c27e, the only change is the import-again busy guard in pi-tui-runner.ts:3314-3323. That guard is correct and tested. The session discovery, filtering and paging code is unchanged, so both P2s from the previous round are still open. I'm now grading one of them P1 because of how often it triggers.
P1: when the scan hits the page cap it throws, and the sessions already found are discarded.
runtime-host-session-driver.ts:396-397throwsMakaSessionCatalogIncompleteErrorwhenever 8 pages have been read and a cursor remains. Pages hold at most 32 entries, and fewer when the byte cap applies, so at most about 256 sessions are scanned.- The picker therefore can't open whenever the current cwd has fewer than 200 matches within the first 8 pages and the catalog is larger than 8 pages. That is the typical state for a heavy user.
- Example: 300 sessions, 20 of them in
/repo, the latest one interrupted./resumeshows only "Use /session…". - The startup resumable hint (
pi-tui-runner.ts:3391-3401) swallows the same error silently. - The new test at
runtime-host-session-driver.test.ts:140asserts this behaviour as intended. - Suggested fix: return the sessions found so far together with an explicit
incompleteflag, and let the UI say "showing the N most recent; there may be more".
P2: the 200 limit is applied before the resumable filter, with no truncation signal. runtime-host-session-driver.ts:381-389 returns normally once limit is hit, and the resumable filter only runs afterwards (pi-tui-runner.ts:3490). If the interrupted session sorts after the 200th candidate, the picker is empty and shows "No matching sessions".
P2: cost per /resume. Each /resume can read up to 8 catalog pages and then issue up to 200 turn.resume.query calls. It repeats that on every scope toggle, plus a scan at startup. On the Host that query passes session admission (root-turn-coordinator.ts:2178), so it isn't a light read. It all runs inside the runControl busy state with no progress indication. Consider batching, or only querying resumability for the visible page.
P2 (partly unverified): retries always restart from page 1. With a running turn changing the catalog revision, all 3 attempts may fail.
P3:
- The startup hint (
pi-tui-runner.ts:5094) can race--resume <id>attach and check the first session in the cwd. Without an attached session, the "This session has an interrupted run" wording is also ambiguous. - Toggling scope inside the
/resumepicker also changes/session's default scope (pi-tui-runner.ts:3592). - For non-host execution locations, the candidate check skips the local cwd existence check (
runtime-host-session-driver.ts:413-420). Sessions whose cwd was deleted are listed and then fail to switch. - "No interrupted sessions" and "every check failed" both render as "No matching sessions".
Verified:
- Built the CLI and its dependencies; ran
pi-tui-runner.testandruntime-host-session-driver.test, 313/313 passing. - No test covers the common case of a few cwd matches with a catalog larger than 8 pages, or
limittruncation.
Not verified: a real Host with a large session store (the cost points come from reading the code), and whether turn.resume.query is fully side-effect free.
Automated review (Claude lineage) by the Qronos review line on behalf of @Astro-Han; key claims were checked against the code, but please verify before acting.
| } | ||
| cursors.add(cursor.cursor); | ||
| } | ||
| if (cursor !== undefined && pagesRead === MAX_SESSION_CATALOG_SCAN_PAGES) { |
There was a problem hiding this comment.
P1: at the page cap this throws and discards sessions already collected. For a heavy user whose cwd has fewer than 200 matches in the first ~256 catalog entries, /resume never opens. Please return the partial list with an explicit incomplete flag instead.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
One additional current-head finding supplements my earlier review: reaching the eight-page global scan bound discards resumable Sessions already found in earlier pages, leaving /resume unusable for that catalog. A production-driver probe with one ready current-workspace Session on page one and a ninth global page throws MakaSessionCatalogIncompleteError after eight pages rather than returning that known candidate. The finding and suggested regression are inline.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| } | ||
| cursors.add(cursor.cursor); | ||
| } | ||
| if (cursor !== undefined && pagesRead === MAX_SESSION_CATALOG_SCAN_PAGES) { |
There was a problem hiding this comment.
[P2] Preserve already-discovered current-workspace candidates when the global eight-page scan bound is hit. With one ready /repo Session on page one and unrelated Sessions filling pages two through nine, this throws after page eight and showSessionList converts the error into an incomplete notice before it can render the known ready Session. I reproduced that using this head's production driver (MakaSessionCatalogIncompleteError, eight pages, known candidate ready). The existing test asserts the exception but not the user-visible loss. Return a partial result with an explicit incomplete state, or otherwise allow the known safe candidates to remain selectable; add a picker regression for this distribution.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current head, including the pagination changes since 2461c10. The previous 200-candidate truncation no longer silently reports a complete result: the TUI requests up to the Host’s eight-page/256-item bound, and the driver raises an incomplete result with the sessions already found if a next cursor remains. The TUI shows an incomplete-catalog notice while retaining those sessions as selectable rows. This also addresses the prior eight-page behavior that discarded known resumable sessions. The added driver/TUI regressions exercise a ready session after 200 unavailable rows, truncation at 257 rows, and a partial result still selectable. I found no substantiated new P0–P3 in this increment.
Verification: Node 24 clean install, repository dependency patches, build:test, and the two focused CLI suites (318/318) pass. Current-head CI test succeeds; the PR diff-check and static merge against current main 0fd7540 are clean. No schema/migration changes. I did not exercise a real remote catalog containing more than 256 entries or native-platform TUI behavior; the bounded scan can still omit later sessions, but it now says that the catalog is incomplete rather than presenting the prefix as exhaustive.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Independent re-review of head 1ce501c9 (Claude lineage), checked against our findings on 2461c100.
The previous P1 is fixed. On hitting the page cap, the error now carries the sessions found so far (session-driver.ts:263-270, runtime-host-session-driver.ts:405-409). The picker shows them with an incomplete notice (pi-tui-runner.ts:3395-3406), and this is covered by tests.
The truncation signal is added, but it can be swallowed (P2).
showSessionCatalogIncompleteNotice(pi-tui-runner.ts:3388-3393) de-duplicates by "a notice with the same text already exists in the transcript".- The startup resume check (
announceResumeAvailability,:3612-3619) now also goes throughreadSessionCatalog. - So for any user whose catalog is larger than 8 pages, the first thing they see at every launch is "Showing the sessions found so far", with no session list on screen. The test at
:6061pins that behaviour. - Because of the de-duplication, a later
/resumethat really is truncated shows no notice at all, and that notice is exactly the signal the truncation fix relies on. - Consider not surfacing the notice from the startup probe (or surfacing a probe-specific one), and scoping de-duplication to the current picker invocation.
Status of the earlier findings:
| Finding | Status | Notes |
|---|---|---|
| P2: truncate before filter | partly fixed | An incomplete signal and partial results now exist, and the limit is 256; the structure is unchanged and the signal can be lost as described above |
P2: cost per /resume |
not fixed | Up to 256 turn.resume.query calls, still inside the busy state |
| P2: retries restart from page 1 | partly mitigated | 8 attempts with backoff (:128-130, :412-427), but it still rescans from page 1 |
P3: startup hint vs --resume <id> race |
wording fixed | The race remains (:5103 vs :5107) |
P3: scope toggle changes /session default |
not fixed | :3601 |
| P3: non-host cwd existence check skipped | not fixed | :437-445 |
| P3: empty vs all-checks-failed indistinguishable | not fixed | :3447-3450 |
New P3:
- Incomplete results aren't cwd-ordered. Under a non-host execution location, the incomplete path returns sessions without the cwd ordering that
listSessionsapplies (runtime-host-session-driver.ts:397/408vs:340-350). This affects display order in the All scope only. - A latent case with no truncation signal. When
limitis reached mid-way through the last allowed page, truncation has no signal (:383-391). This is unreachable with today's constants (256 = 8 × 32), but it will surface if the limit or page size changes.
Tests run: pi-tui-runner and runtime-host-session-driver tests (318/318) and check-tui-copy pass.
Not verified: a real Host with a large catalog (the every-launch notice is inferred from the code and the test at :6061), and whether the new tests fail on the old head.
Automated review (Claude lineage) by the Qronos review line on behalf of @Astro-Han; key claims were checked against the code, but please verify before acting.
| const sessionTree = projectRevisionLinkedSessionTree( | ||
| const showSessionCatalogIncompleteNotice = (): void => { | ||
| const text = pickerCopy.resumeCatalogIncompleteNotice; | ||
| if (state.entries.some((entry) => entry.kind === 'notice' && entry.text === text)) return; |
There was a problem hiding this comment.
P2: this de-duplicates against the whole transcript. The startup probe (announceResumeAvailability) also calls readSessionCatalog, so a large catalog shows this notice at launch with no list on screen. A later, genuinely truncated /resume then shows nothing. Consider keeping the probe from emitting it, and scoping the de-duplication to the current picker.
Generated-by: Codex
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed current head 9a84c7c9594eef7f38a186e74e8cdc557db34ac1. The two previously reported P2 catalog-boundary failures are addressed. packages/cli/src/pi-tui-runner.ts:541-545 now checks all 256 entries covered by the bounded eight-page Host scan, rather than silently stopping readiness checks at 200. packages/cli/src/runtime-host-session-driver.ts:386-406 attaches the sessions already found to an incomplete-catalog error, and pi-tui-runner.ts:3385-3420 keeps those rows selectable while showing an incomplete-results notice. The new driver and TUI tests cover a ready 201st session and selection of a known row when discovery exceeds eight pages. I found no additional substantiated P0–P3 code issue in this increment.
Node 24 clean install, build:test, and the two focused CLI suites passed (318 tests). git diff --check and a local merge-tree against fresh main de4fc5ff95b1f8034ca00b448984b31ca711dce1 are clean. No schema or migration changes are introduced.
The current-head hosted test check is RED, so this is not merge-ready. Its log shows checkout of the PR merge commit 2e1663623 against main 8b104db12, then failure in npm run windows:inventory with “Windows test skip inventory is stale.” The inventory check passes on the unmerged PR head (100 declarations); this distinguishes the merge-context failure from the changed CLI tests, which also pass locally. Refresh the generated inventory for the merged tree and rerun CI. I did not run a real remote Host with a large catalog or native Windows/macOS TUI.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
A second, independent review of head 9a84c7c9 (Claude lineage), alongside the hqhq1025 review 5335188588. We found no P0–P2.
The P2 we left open last round is fixed. The startup scan (announceResumeAvailability, pi-tui-runner.ts:3620-3630) no longer emits the incomplete notice. The transcript-wide dedupe is gone, and each /resume now uses its own shown-once flag (:3408-3416, :3590), so every incomplete /resume shows the notice. The startup test (pi-tui-runner.test.ts:6061-6108) covers both behaviours.
Partial sessions are safe to select. Partial results keep the same shape as full ones, still pass the per-session resumable check, and resume through the normal goToSession → resumeSession path. Candidates past the 256 cap are not silent drops any more, because the driver raises the incomplete signal (runtime-host-session-driver.ts:393-399).
P3 (new to us; present before this round): a failed session-list query blocks the next one. pi-tui-runner.ts:3030 chains sessionListPromise.promise.then(() => listSessions(options)) with no rejection handler. If a query with different options starts while an earlier one fails, the second never runs and inherits the first one's error. For example, with a catalog over 8 pages, /session typed during the startup scan shows only the current-directory partial list. .then(run, run) would fix it.
CI: the red test check comes from a merge commit against an older main whose Windows skip inventory was stale, which #5779 has since fixed. We merged this head into current main locally: it merges cleanly, and node scripts/windows-test-inventory.mjs --check passes (118 declarations). The PR adds no Windows skips. Merging main into the branch, or rebasing onto it, should turn the check green.
Tests: the touched CLI tests pass 318/318, and check-tui-copy passes.
Automated review (Claude lineage) by the Qronos review line on behalf of @Astro-Han; please verify before acting.
me2seeks
left a comment
me2seeks
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent operated by me2seeks make. It is not an independent human review and does not replace one.
Summary
Discovers resumable sessions from /resume: goToSession now returns a success boolean (with side-pair discard only on actual switch and errors reported instead of swallowed), resume availability is probed with concurrency 8, and MAX_SESSION_RESUME_CANDIDATES=256 is chosen to match the Runtime Host's eight-page, 32-item catalog scan bound — the comment documents why, so readiness checks can't silently stop before the end of the bounded result. Session-driver, pickers, and the TUI copy catalog move in step, with a check-tui-copy update.
Findings
- [P1]
testfails on this head — the required merge gate is red. For a/resumediscovery change, the failing lane is very likely the pi-tui-runner or session-driver suite; pull the log, fix, and show green before merge. - [P3] The 256 candidate bound is derived from the Host's 8×32 catalog scan by hand — if the Host's scan bound changes, this constant silently disagrees. A shared constant or a comment reference would keep them in lockstep (the comment exists; a single authority would be better).
Verdict
needs-changes — correct discovery semantics with careful error surfacing, but the red test gate must be fixed first.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed current head 5ef0548468f1cff5662ea8a569ff1c4a8a36fbed. The follow-up derives the resume picker’s 256-session candidate bound from the Runtime Host catalog’s eight-page bound and protocol page size, and refreshes the Windows test-skip inventory. The previously reported merged-tree inventory failure is resolved. I found no new substantiated P0–P3 issue in this increment.
The exact GitHub merge commit 87f35b679a831a2c6f8ce36daeac8a84ef4e9477 (main 2f32205528a4436783c800715261d4b6e286023d) passes npm run windows:inventory with 118 declarations; its diff check is clean, and the current-head hosted test check succeeds. On Node 24, build:test, both focused CLI suites (318/318), and the TUI copy check pass. The raw PR head alone reports a stale inventory because it does not include the newer tests on main; the merge tree that CI evaluates passes.
I did not manually exercise native Windows/macOS TUI behavior or a real large remote catalog. This COMMENTED review is not approval to merge.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Incremental re-review at 5ef05484 (Claude lineage). Our previous full review (9a84c7c9) found no blocking issues. The only new commit is 5ef054846 (4 files, +58/−17). Verdict: no P0–P3 in the delta.
- The new
session-catalog-limits.tshoists the 8-page scan cap and the derived candidate bound into one module. runtime-host-session-driver.ts:374,405and the picker inpi-tui-runner.tsnow share it, so the driver's scan bound and the picker's candidate bound can no longer drift apart.- The loop and the incomplete-catalog check keep identical semantics.
- The refreshed Windows skip inventory fixes the stale-merge-tree failure that previously kept CI red. Hosted
testis now green.
Automated review (Claude lineage, posted from the Astro-Han account). No approval implied.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review: 5ef05484 (our last reviewed head, no P0-P3 in that delta) to 8e584c03 (three new commits: 12fbac35 "retry session list after catalog failure", b36636b4 "remove unrelated inventory from resume PR", and 8e584c03 "Revert "fix(cli): guard external imports during active turns""). The merge-base with main is unchanged (54542021), so the delta is exactly 5ef05484..8e584c03 (3 files, +61/-100). GitHub reports MERGEABLE (merge state BLOCKED). Hosted CI test is green on this head. The PR touches no protocol files, so there is no epoch impact.
Prior findings.
- The open P3 from
9a84c7c9(a failed shared session-list query blocking the next one) is fixed.pi-tui-runner.ts:3028-3031now chains with.then(run, run), and the new test/session continues after a concurrent startup catalog scan failscovers it. - The Windows inventory refresh was dropped from the PR. I checked the merge of this head into current main
dcef6427: it merges cleanly, andnode scripts/windows-test-inventory.mjs --checkpasses (120 declarations). Dropping it does not break CI. - The P3s still open from
1ce501c9are not touched by this delta: the scope toggle changes/session's default scope, the non-host cwd existence check is skipped, and "no candidates" looks the same as "all checks failed".
Delta findings.
- P3 (inline, re-opened): the revert removes the inner
busy || turnRunningguard on "Import again" again (pi-tui-runner.ts:3314). That guard exists on main (main:3281-3290), so this is the PR changing unrelated external-import behavior, not removing something unrelated it had added. The outer guard at:3294still covers the state when the action picker opens. The gap is a Turn or control action that starts while the second picker is open: "Import again" then either does nothing without a message (busy) or imports during a running Turn. If the goal is to keep the PR out of the import path, restore main's code here rather than reverting the restoration. - P3 (still open): the PR still deletes main's test
opens the latest imported task without importing another copy. It is present on main and absent on this head. It covers this same picker, so it should come back along with main's import code.
No P0-P2 in this delta.
Verification and limits. I read the delta at this exact head and checked the import region against main and the merge-base. I built a temporary merge with current main: no conflicts, git diff --check clean, Windows inventory check passes. The local TUI-copy check could not run because of an environment problem: the shared node_modules is missing @babel/parser, and the check fails the same way on main. Hosted CI test, which includes it, is green. I did not run the CLI suites locally.
| requestRender(); | ||
| return; | ||
| } | ||
| if (action.value === 'import-again') { |
There was a problem hiding this comment.
P3 (re-opened by the revert): this reverts the restoration of main's inner guard, not an addition unrelated to resume. Main has if (busy || turnRunning) { ...externalImportBusy...; return; } right here (main pi-tui-runner.ts:3281-3290), so this head still changes external-import behavior. If a Turn or control action starts while this second picker is open, "Import again" either returns silently from runControl (busy) or imports while the Turn is running. The outer guard at :3294 only checks state when the first picker closes. To keep this PR out of the import path, restore main's block (and main's deleted test opens the latest imported task without importing another copy) rather than dropping it.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review: 8e584c03 (our last reviewed head) to fa68e24a. There is one new commit: fa68e24a "chore: remove unrelated picker changes from resume PR". The merge-base with main is still 54542021, so the delta is exactly 8e584c03..fa68e24a. It touches only packages/cli/src/pi-tui-runner.ts (+63/-20). Comparing the old head against its merge-base with the new head against its merge-base, the PR-vs-base diff of pi-tui-runner.ts shrinks from +344/-117 lines to +261/-54. The other 8 files are unchanged. GitHub reports MERGEABLE (merge state BLOCKED). Hosted CI test is green on this head. The PR touches no protocol files, so there is no epoch impact.
Prior findings.
- Fixed: the P3 about the missing
busy || turnRunningguard on "Import again".pi-tui-runner.ts:3343-3351restores main's inner guard. The rest of the delta reverts the PR's reformatting ofimportExternalSession,showExternalSessionPageandshowExternalSourcePicker. The PR diff against the merge-base now has no hunks between base lines 2997 and 3365, so the external-import path is byte-identical to main. - Corrected and downgraded: the P3 "PR deletes main's test
opens the latest imported task without importing another copy". That was inaccurate. The test body is still there atpi-tui-runner.test.ts:6563and matches main's body exactly (I diffed them). It runs under the titlesurfaces a notice when the foreign-session scan fails. That was this test's old title before main's #5308 (84266767) replaced the body and renamed it. The PR's merges from main kept #5308's body but not its new title, and every PR commit back to94df1467has the stale title. Coverage is intact. What remains is a misleading test name (see inline). It is a nit, P3 at most. - The P3s still open from
1ce501c9are not touched by this delta: the scope toggle changes/session's default scope, the non-host cwd existence check is skipped, and "no candidates" looks the same as "all checks failed".
Delta findings. None at P0-P2. The delta restores main's code: the guard, the error-code mapping, the comment and the formatting. There is no new behavior to review.
Verification and limits. I diffed old head vs merge-base against new head vs merge-base, and checked the import region and the test body against origin/main (dcef6427). I ran git merge-tree of this head with current main: it merges cleanly, and the merged tree has the same stale test title. I did not run the CLI suites locally. Hosted CI test is green.
| await run; | ||
| }); | ||
|
|
||
| test('surfaces a notice when the foreign-session scan fails', async () => { |
There was a problem hiding this comment.
P3 (nit, test name): this is main's opens the latest imported task without importing another copy test. The body is identical to main's, but it carries the title main used before #5308 (84266767) replaced the body. Nothing here touches a foreign-session scan failure; the test opens the latest import and checks that importSession is never called. Please restore main's title so the PR does not rename an unrelated test and the suite name matches what it covers.
Generated-by: Codex
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review: fa68e24a (our last reviewed head) to 6a9110d0. There is one new commit, 6a9110d0 "test(cli): restore imported task test title". The merge-base with main is still 54542021, and comparing old head vs merge-base against new head vs merge-base shows a one-line delta in packages/cli/src/__tests__/pi-tui-runner.test.ts.
Prior findings.
- Fixed: the P3 stale test title.
pi-tui-runner.test.ts:6563is again titledopens the latest imported task without importing another copy, which matches main's title and the test body. The PR no longer renames an unrelated test. - The older P3s from
1ce501c9are not touched by this delta: the scope toggle changes/session's default scope, the non-host cwd existence check is skipped, and "no candidates" looks the same as "all checks failed".
Findings. None at P0-P2, and no new P3. There are no protocol files, so there is no epoch impact.
Status: MERGEABLE (merge state BLOCKED). It merges cleanly with current main 3597abe8 (git merge-tree). Hosted CI test is green on this head.
| session: SessionSummary, | ||
| ): Promise<SessionResumeAvailability> { | ||
| if (!session.cwd) return { available: false, reason: 'Missing working directory' }; | ||
| const plan = await this.#request('turn.resume.query', { sessionId: session.id }); |
There was a problem hiding this comment.
[P3] /resume can list a local session whose working directory no longer exists
For a local (client_path) session, this candidate check only asks the Host whether turn.resume.query is ready; it does not apply the existing local cwd existence check. I reproduced the mismatch on the current-main merge tree: with a deleted cwd and a ready resume plan, /resume marks the row available, while switchSession() rejects it with Session cwd no longer exists. Selecting that row therefore closes the picker without starting the resume. The switch guard still prevents attaching to the missing directory.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review: 6a9110d0 (our last reviewed head) to 7e2d0f55. There is one new commit, 7e2d0f55 "fix(cli): check local cwd before resume discovery" (+81/-1 across 2 files). The merge-base with main is still 54542021. Comparing old head vs merge-base against new head vs merge-base shows only that commit's changes in runtime-host-session-driver.ts and its test.
Prior findings.
- Fixed: the P3 about the skipped non-host cwd existence check.
getSessionResumeCandidateAvailability(packages/cli/src/runtime-host-session-driver.ts:437-449) now callsinspectRuntimeHostSessionResumeAvailabilityfirst. That is the same checkgetSessionResumeAvailabilityandassertSessionResumeAvailableuse. Forclient_pathsessions it checks the local filesystem and returnsWorking directory no longer existsbefore sendingturn.resume.query. Forhostexecution it still trusts the Host, which is correct because the path is not local. The new test (runtime-host-session-driver.test.ts:468) covers all three cases: a missing local cwd (no RPC sent), a present local cwd (one RPC), and a remote host path (one RPC, no local check). - Still open (P3, non-blocking, unchanged): the scope toggle changes
/session's default scope, and "no candidates" looks the same as "all checks failed".
Findings. None at P0-P2, and no new P3. There are no protocol files, so there is no epoch impact.
Status: MERGEABLE (merge state BLOCKED, waiting on review). It merges cleanly with current main 3597abe8 (git merge-tree). Hosted CI test passes on 7e2d0f55.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review: 7e2d0f55 → 055c7dc0 (one commit, "fix(cli): isolate resume picker scope and failures"). The merge-base is unchanged (54542021).
Both remaining P3s from earlier rounds are fixed:
- Scope leak: toggling scope inside the
/resumepicker no longer rewritessessionListScope. The guardif (!options.onlyResumable)is inpi-tui-runner.ts, so/sessionkeeps its own default scope. - "No candidates" vs "all checks failed": the runner now tracks sessions whose candidate check threw, or that have no driver support, in
resumeCandidateCheckFailures. When every visible session failed, the picker showsresumeCandidateCheckFailedinstead of "No matching sessions". The overlay now carriesemptyTextthroughupdateChoices, so the message survives a scope toggle. The new string exists in en, zh-CN and zh-TW. A session that is merely unavailable (for example, a missing cwd) is correctly not counted as a failure.
The new tests in pi-tui-runner.test.ts cover both behaviours. No new findings, CI test is green, and the PR is mergeable.
This branch has not been deployed
4 participants
Summary
Fixes #3508: #3508
The TUI now surfaces a passive hint when the current or attached session has a safe-boundary resume available. Running /resume without an attached session opens a picker scoped to the current working directory, showing only sessions the Runtime Host reports as resumable. Selecting a row switches to that session and resumes it.
Implementation
Verification
Not run: the full npm test workspace suite, Docker-based checks, and Windows platform CI.
AI use
Tool(s) and scope: Codex assisted with implementation and regression testing. The implementation commit includes a Generated-by: Codex trailer.
Checklist
Does this PR entail a change in behavior?