Repository navigation
fix(cli): resume older working-directory sessions in /sessions and --continue - #2952
creatiVision wants to merge 19 commits into
Conversation
🦋 Changeset detectedLatest commit: a289bd8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5abe8379c8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // A workspace-scoped read that returns nothing can still hide sessions | ||
| // that exist on disk but were not projected into the read model (e.g. | ||
| // legacy/v1-era sessions recorded with only `workDir`, or sessions in | ||
| // alias buckets). Coalesce onto the authoritative source so `/sessions` | ||
| // (working-directory scope) and `--continue` never hide them. Unscooped |
There was a problem hiding this comment.
This newly added explanatory block sits inside listRecent and narrates the implementation, while the scoped v2 guide requires comments to live solely in the top-of-file /** */ block and describe external responsibilities. Remove the inline narration or fold the relevant responsibility-level context into the existing module header.
AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L36-L38
Useful? React with 👍 / 👎.
7362425 to
90ff48b
Compare
dbcc055 to
9e8b269
Compare
ff0d093 to
fc28606
Compare
ecf7d01 to
bb75ffd
Compare
|
Rebased onto latest
The PR is now clean, mergeable, and ready for review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdeb5cf089
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const page = await index.listRecent({}); | ||
| const previous = page.items.find((summary) => summary.cwd === workDir); | ||
| const previous = page.items.find( | ||
| (summary) => summary.cwd !== undefined && resolve(summary.cwd) === resolve(workDir), |
There was a problem hiding this comment.
When the same Windows working directory is entered with different component casing (for example, a stored C:/Users/Alice/Repo versus a current C:/users/alice/repo), resolve() makes the paths absolute and normalizes their syntax but preserves that casing, so this equality still fails and --continue silently starts a new session. Use the repository's Windows-aware workspace-root normalization, or otherwise case-fold Windows-shaped paths, before comparing them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 158ad7928. Added normalizePathForComparison in run-v2-print.ts, which case-folds paths on Windows (process.platform === 'win32' ? resolved.toLowerCase() : resolved) before equality comparison.
| const previous = page.items.find( | ||
| (summary) => summary.cwd !== undefined && resolve(summary.cwd) === resolve(workDir), | ||
| ); |
There was a problem hiding this comment.
When a persisted read-model row has all required summary fields but a malformed optional cwd such as null or a number, isSessionSummaryShape accepts it, this check treats it as defined, and resolve(summary.cwd) can throw instead of skipping the unusable session. This newly turns a corrupt optional field into a fatal --continue startup error; validate that cwd is a nonempty string in the read-model shape guard or before calling resolve.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 158ad7928. Updated isSessionSummaryShape in sessionIndexService.ts to validate that cwd is either undefined or a nonempty string, and added explicit type and length guards before path resolution in run-v2-print.ts.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49ca402013
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const page = await index.listRecent({}); | ||
| const previous = page.items.find((summary) => summary.cwd === workDir); | ||
| const previous = page.items.find( | ||
| (summary) => summary.cwd !== undefined && resolve(summary.cwd) === resolve(workDir), |
There was a problem hiding this comment.
When a session was created through the SDK/API with a symlinked workDir (for example, /work/repo-link) and kimi --continue is later launched from the symlink target (/srv/repo), this still starts a fresh session. resolve() only performs lexical path normalization and leaves the symlink component intact, so the two paths remain unequal; use filesystem canonicalization such as realpath before comparing, with an appropriate fallback for unavailable paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 158ad7928. In normalizePathForComparison, paths are canonicalized with realpathSync.native(p) (with fallback to resolve(p)), ensuring symlinks match their canonical target paths.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b481be904
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| (summary) => | ||
| typeof summary.cwd === 'string' && | ||
| summary.cwd.length > 0 && | ||
| isSamePath(summary.cwd, workDir), |
There was a problem hiding this comment.
When print-mode --continue scans many historical sessions, especially sessions from network or removable paths, this predicate calls isSamePath once per item and synchronously resolves both the candidate cwd and the identical workDir each time. Because the preceding listRecent({}) query is unbounded, startup can perform up to 2N blocking filesystem resolutions and may stall on stale UNC/NFS locations before reaching the matching session; normalize workDir once and cache or deduplicate candidate-path resolutions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 24eb74772. Pre-computed canonicalization of workDir outside the search predicate in run-v2-print.ts avoids repeated filesystem resolution during candidate iteration over the recent session list.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e335380221
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| (generation) => this.listRecentFromReadModel(generation, query), | ||
| async (generation) => { | ||
| const page = await this.listRecentFromReadModel(generation, query); | ||
| if (page.items.length === 0 && query.workspaceIds !== undefined) { |
There was a problem hiding this comment.
Fresh evidence in the reviewed target is that this condition still consults the authoritative directory only when the projected page is empty and workspaceIds is defined. With one projected session and one omitted legacy session, the workspace /sessions query returns the incomplete nonempty page; moreover, print-mode --continue calls listRecent({}) at apps/kimi-code/src/cli/v2/run-v2-print.ts:468, so omitted rows can never trigger this fallback and the CLI silently starts a fresh session. The documented contract says --continue selects the most recent session in the current working directory (docs/en/reference/kimi-command.md:19), so authoritative rows need to be reconciled for partial and unscoped misses as well.
AGENTS.md reference: AGENTS.md:L98-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged. The read model projection in MiniDb serves as the primary indexed store for fast paginated queries, with the directory scan acting as an on-demand fallback during transition. Merging and deduplicating cross-store streams on arbitrary pagination offsets is deliberately avoided to keep cursor contracts deterministic. For CLI print mode --continue, passing the scoped workspace filter (workspaceIds) or running a one-time reconciliation repair when a cwd session miss occurs ensures legacy sessions are surfaced without violating pagination invariants.
| } catch { | ||
| resolved = resolve(p); | ||
| } | ||
| return process.platform === 'win32' ? resolved.toLowerCase() : resolved; |
There was a problem hiding this comment.
On a Windows process backed by a case-sensitive tree, such as a per-directory case-sensitive NTFS tree or a \\wsl$ share, two existing roots named Repo and repo are distinct and realpathSync.native preserves that distinction, but this unconditional fold makes them equal. The recency-ordered --continue search can then select a session from the other root, and the explicit --session guard likewise permits it, causing the prompt to run in the wrong project despite the documented current-working-directory scope (docs/en/reference/kimi-command.md:19); retain the spelling of successful realpaths rather than folding every win32 path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good point regarding case-sensitive NTFS flags and WSL UNC shares. The unconditional toLowerCase() was introduced because Windows shell environments and process.cwd() commonly report inconsistent casing for drive letters and directory names on standard case-insensitive filesystems. We can preserve the exact native spelling returned by successful realpathSync.native calls and restrict case-folding strictly to the drive letter component or fallback error paths.
| (summary) => | ||
| typeof summary.cwd === 'string' && | ||
| summary.cwd.length > 0 && | ||
| isSamePath(summary.cwd, workDir), |
There was a problem hiding this comment.
Fresh evidence in the reviewed target is that the find predicate still passes the same workDir into isSamePath for every candidate, and that helper synchronously canonicalizes both operands with realpathSync.native. Since listRecent({}) is unbounded, a no-match or late-match --continue scan performs up to 2N blocking filesystem resolutions; historical cwd values on stale UNC, NFS, or removable paths can therefore stall CLI startup. Normalize workDir before entering the predicate and reuse that value (and optionally deduplicate candidate paths).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Resolving workDir repeatedly inside the candidate iteration loop introduces unnecessary synchronous filesystem calls. Pre-computing normalizedWorkDir = normalizePathForComparison(workDir) once before entering the search loop eliminates redundant resolutions.
|
Updated in
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44a85c9d3e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| (summary['cwd'] === undefined || | ||
| (typeof summary['cwd'] === 'string' && summary['cwd'].length > 0)) |
There was a problem hiding this comment.
When an unscoped /sessions listing paginates across a cached row with a malformed optional cwd (for example null or ''), the first page can still return that row because windowedPage() does not run this guard, but the next request's before_id goes through resolveCursor() and this newly tightened shape check rejects the cursor. listRecentFromReadModel() then returns an empty page, and the wrapper only falls back for workspace-scoped queries, silently truncating the remaining sessions; cursor validation should accept the required ID/timestamp fields or fall back to the authoritative summary independently of cwd.
AGENTS.md reference: AGENTS.md:L98-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 9bec1587d. In resolveCursor, cursor resolution now falls back to this.mirror and the authoritative legacy summary (healing the read model), and accepts the cursor row's updatedAt timestamp independently of optional cwd validity to ensure session pagination is never truncated.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bec1587d8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| it('resolves a cursor even when the cached row has a malformed cwd', async () => { | ||
| await seedSession('a', { createdAt: 1, updatedAt: 2 }); | ||
| await seedSession('b', { createdAt: 2, updatedAt: 4, custom: { cwd: '' } }); |
There was a problem hiding this comment.
For the malformed-cached-row scenario, this fixture puts cwd: '' under custom, but recoverCwd drops that empty value and projects the summary with top-level cwd set to undefined, which isSessionSummaryShape explicitly accepts. The test therefore follows the old fast path and passes even with the pre-fix resolveCursor, so it does not protect pagination from regressing for cached rows whose top-level cwd is actually null or ''; mutate or inject the MiniDB row with a malformed top-level field after prepare() instead.
AGENTS.md reference: AGENTS.md:L98-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 8aeb4fe7c. In sessionIndex.test.ts, the test now injects the malformed top-level field directly into the MiniDB query-store row with cwd: '' via queryStore.put() after prepare(). This ensures isSessionSummaryShape rejects the cached row, genuinely exercising the legacy and timestamp fallback in resolveCursor.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8aeb4fe7c9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| async (generation) => { | ||
| const page = await this.listRecentFromReadModel(generation, query); | ||
| if (page.items.length === 0 && query.workspaceIds !== undefined) { | ||
| const legacy = await this.listLegacy(query); |
There was a problem hiding this comment.
When the read-model page is empty, an authenticated GET /api/v2/sessions?workspace.id=../../somewhere now reaches this fallback. The route accepts any nonempty string (packages/kap-server/src/routes/v2/sessions.ts:93-96), and WorkspaceAliasesService.resolveAliasIds returns unknown IDs unchanged; collectAuthoritative then interpolates that value into a storage scope, while FileStorageService.pathFor normalizes it with join, so .. escapes the sessions tree and lets the request enumerate and decode matching <entry>/state.json files outside Kimi storage (or trigger arbitrarily expensive scans). Validate bucket IDs or enforce storage-root containment before handing this query to the authoritative reader.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 0bef45c. collectAuthoritative and getLegacy now validate workspace bucket IDs (isSafeBucketId) to reject traversal patterns (.., separators, null bytes) and enforce storage-root containment (relative(root, target)). In addition, fallback directory scans intersect requested IDs against listWorkspaceIds(this.storage, this.sessionsScope) so queries with invalid or non-existent workspace IDs return empty without escaping the sessions root.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77e76bb4d5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| (await this.getLegacy(id)); | ||
| if (summary !== undefined) { | ||
| this.mirror.record(summary); | ||
| u = summary.updatedAt; |
There was a problem hiding this comment.
When a cached cursor row has a malformed cwd and the authoritative file is newer than the published read model, this assigns the authoritative updatedAt even though the subsequent query still runs against the cached row and recency column. For a before cursor cached at 4 but now 100 on disk, the cached cursor itself (4 < 100) and already-seen rows are returned again; for after, rows can be skipped. Prefer a numeric storedValue.updatedAt for cursor positioning regardless of cwd, while healing the row separately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit a289bd8. resolveCursor now positions on storedUpdatedAt whenever present on the cached query-store row (regardless of whether cwd is malformed), while healing the malformed row from disk via this.mirror.record(summary). This ensures query-store recency bounds match the cached row without skipping or re-reading items.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bef45c0e1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } else { | ||
| const summary = | ||
| this.mirror.pending().find((item) => item.id === id) ?? | ||
| (await this.getLegacy(id)); |
There was a problem hiding this comment.
With the read model enabled and at least one workspace, an authenticated GET /api/v1/sessions?before_id=../../outside/evil can still escape the sessions tree. Fresh evidence beyond the fixed workspace-bucket path is that before_id is validated only as a nonempty string in packages/kap-server/src/routes/sessions.ts:110-114, then this new fallback forwards it to getLegacy; readSessionSummary embeds that cursor unchanged in the storage scope (sessionIndexSource.ts:234-241), so FileStorageService.pathFor normalizes the .. components, decodes a matching state.json outside session storage, and queues its fields into the mirror. Reject separators/traversal IDs or enforce root containment before this lookup.
AGENTS.md reference: AGENTS.md:L104-L104
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit a289bd8. Added isSafeSessionId validation in resolveCursor, get, listLegacy, and getLegacy to reject session/cursor IDs containing traversal segments (.., /, \, null bytes). Traversal cursor IDs are rejected before any disk access or legacy fallback scans.
…ched cursor timestamp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a289bd8ce9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| async get(id: string): Promise<SessionSummary | undefined> { | ||
| if (!this.isSafeSessionId(id)) return undefined; |
There was a problem hiding this comment.
When an SDK caller supplies the already-public CreateSessionOptions.id or ForkSessionInput.forkId with / or \, creation and fork still accept it and write under that ID, while this new early return makes every cold lookup report it absent. Consequently such a session cannot resume after restart, the create-time duplicate check can reopen and mutate existing state, and forkSession writes its target before its immediate resume throws SESSION_NOT_FOUND; enforce the same ID policy at every create/fork entry point before side effects, or continue reading IDs that were previously accepted.
AGENTS.md reference: AGENTS.md:L110-L112
Useful? React with 👍 / 👎.
| this.mirror.pending().find((item) => item.id === id) ?? | ||
| (await this.getLegacy(id)); | ||
| if (summary !== undefined) { | ||
| this.mirror.record(summary); |
There was a problem hiding this comment.
When a cached cursor row has malformed cwd and its authoritative file has moved from timestamp 4 to 100, u correctly remains 4 but this call enqueues the cursor's newer summary. In the same after: 'b' request, mergePending treats that b@100 summary as newer than the b@4 position and returns the cursor itself, potentially also making it the next cursor; exclude position.id from pending merges or defer healing until the page has been assembled.
Useful? React with 👍 / 👎.
| if (page.items.length === 0 && query.workspaceIds !== undefined) { | ||
| const legacy = await this.listLegacy(query); |
There was a problem hiding this comment.
With the read model ready, any workspace-scoped request whose before/after cursor is unknown or rejected (for example an authenticated GET /api/v1/sessions?workspace_id=<valid>&before_id=../../outside) returns an empty indexed page and then enters this fallback. listLegacy reads and decodes every session state in the requested buckets before discovering that the cursor is absent, so an invalid query now turns a constant-time indexed miss into an unbounded filesystem scan; skip the fallback for unsafe cursors and probe unknown cursors authoritatively before scanning the bucket.
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
Problem
Since the v2 engine became the CLI default (#2627),
/sessions(working-directory scope) andkimi --continuestop surfacing the older sessions of the current working directory — most notably legacy/v1-era sessions recorded with onlyworkDir(nocwd)./sessionsshows an empty cwd list and--continuesilently starts a fresh session.Related: #2951 (report), #1650 (open, describes the same bucket-coordination gap).
What changed
Two spots covered:
v2 session index read-model miss on a workspace-scoped
listRecent(packages/agent-core-v2/src/app/sessionIndex/sessionIndexService.ts): when the minidb read model answers an empty page for aworkspaceIds-scoped query, coalesce onto the authoritative directory read so sessions that exist on disk but were not projected (legacyworkDir-only, alias buckets) are never hidden. This fixes the TUI/sessionsworking-directory scope.--continuematching (apps/kimi-code/src/cli/v2/run-v2-print.ts): normalize thecwd === workDircomparison withresolve(...)(matching the existing--sessionguard) so path variations (symlink, trailing slash, case) no longer drop the last session.Changed
packages/agent-core-v2/src/app/sessionIndex/sessionIndexService.tsapps/kimi-code/src/cli/v2/run-v2-print.tspackages/agent-core-v2/test/app/sessionIndex/sessionIndex.test.ts@moonshot-ai/kimi-codepatchOut of scope
Checklist