fix: VS Code extension ignores non-primary folders in multi-root workspaces - #3229
fix: VS Code extension ignores non-primary folders in multi-root workspaces#3229ojassharma7 wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 8f75815 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: a4f5189cf3
ℹ️ 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".
|
|
||
| /** Other multi-root workspace folders, passed to the session as ephemeral additionalDirs. */ | ||
| private get additionalWorkspaceDirs(): readonly string[] { | ||
| return vscode.workspace.workspaceFolders?.slice(1).map((folder) => folder.uri.fsPath) ?? []; |
There was a problem hiding this comment.
When the agent edits a file in one of these newly permitted secondary folders, captureFileBaseline rejects the absolute path because it requires every captured file to be contained by session.workDir. The edit therefore succeeds but never appears in the extension's File Changes list and cannot be restored through its keep/undo or fork-baseline flows. Extend baseline and file tracking to understand the additional workspace roots before exposing them as writable session directories.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8f75815 — good catch, and it turned out to be three containment gates rather than one:
captureFileBaselinedropped these beforeBaselineManagerever saw them. Its resolver returnsundefinedoutsideworkDir, so additional-root files are now resolved from the absolute path and checked against each root.BaselineManager.resolveSessionFilerejected them too. Files under an additional root are keyed in the manifest by absolute path — aworkDir-relative key would escape the root (../other/a.ts) and could collide with a same-named file in another root.paths.resolve(root, <absolute>)returns an absolute path unchanged, so the key round-trips on read and no manifest version bump is needed.requireContainedRestorePath, the realpath-based symlink guard on undo, only knewworkDir. It now takes the session's full root list and accepts a path contained by any of them, still resolving both sides throughrealpathso a symlink can't point a restore outside them. This one surfaced from the new test's undo assertion rather than from reading the code.
Added coverage for capture → File Changes → undo under a secondary root, plus a regression test that a path outside both the workspace and its additional roots is still rejected.
| model, | ||
| effort, | ||
| yoloMode: VSCodeSettings.yoloMode, | ||
| additionalDirs: this.additionalWorkspaceDirs, |
There was a problem hiding this comment.
If a user adds another folder to the VS Code workspace after starting a conversation, subsequent prompts compute the updated additionalDirs here, but KimiRuntime.openSession returns the existing runtime through its same-session fast path and applySessionSettings does not apply this field. Because the extension also has no onDidChangeWorkspaceFolders handler, that conversation continues to reject the newly added root until it is detached or the window is reloaded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8f75815 — confirmed exactly as described: applySessionSettings only touched permission flags, and openSession's same-session fast path returns the existing runtime without re-running create/resume, the only two places additionalDirs was sent.
applySessionSettings now also syncs additionalDirs, using the session's own addAdditionalDir(dir, { persist: false }) (the same call /add-dir uses, ephemeral to match how they're passed at create/resume) and skipping any the session already has, so it's a no-op on the create/resume paths where they were just sent.
I went this route rather than adding an onDidChangeWorkspaceFolders handler since every prompt already recomputes additionalDirs and flows through openSession — this makes that existing path actually apply them, without a second source of truth. Happy to add the event handler too if you'd prefer folders to propagate the moment they're added rather than on the next prompt.
Covered by two tests: a folder added mid-conversation reaching the open session, and no re-add for one it already has.
… sessions Addresses both review findings on this PR. Passing the secondary multi-root folders as additionalDirs made them writable, but two paths downstream still assumed a session only ever writes under its workDir. Track edits made under the added roots. Three separate containment gates rejected these files, each found by following the failure rather than assuming one fix was enough: - bridge-handler's captureFileBaseline dropped them before the baseline manager saw them. Its resolver returns undefined outside workDir, so additional-root files are now resolved from the absolute path and checked against each root. - BaselineManager.resolveSessionFile rejected them too. Files under an additional root are keyed in the manifest by absolute path: a workDir-relative key would escape the root (`../other/a.ts`) and could collide with a same-named file in another root, and `paths.resolve(root, <absolute>)` returns it unchanged, so the key round-trips on read with no manifest version bump. - requireContainedRestorePath, the realpath-based symlink guard on undo, only knew workDir. It now takes the session's full root list and accepts a path contained by any of them - still resolving both sides through realpath, so a symlink cannot point a restore outside them. This one was caught by the new test's undo assertion, not by reading. Refresh additional roots for an already-open session. openSession's same-session fast path returns the existing runtime without re-running create/resume, the only two places additionalDirs was sent, and applySessionSettings only touched permission flags. It now also syncs additionalDirs via the session's own addAdditionalDir(persist: false), skipping any the session already has, so a folder added mid-conversation no longer waits for a detach or window reload. The kimi-runtime fake gained addAdditionalDir so it still matches the Session surface the runtime uses. Verified in a Node 24 container (this repo requires >=24.15.0): tsc --noEmit clean, oxlint 0 errors on the changed files (its 2 warnings are pre-existing console statements in bridge-handler.ts, confirmed against the unmodified file), and 331/332 vscode tests pass. The one failure, baseline.manager's "rejects an unreadable original", is pre-existing and unrelated - it chmods a file to make it unreadable, which does nothing as root in a container; confirmed by running it on the unmodified code.
|
Withdrawing this, see discussion on #3221. |
1 participant
Fixes #3221.
What changed
apps/vscode/src/bridge-handler.tsapps/vscode/src/runtime/kimi-runtime.tsapps/vscode/test/bridge-handler.test.tsapps/vscode/test/kimi-runtime.test.ts.changeset/multi-root-workspace-dirs.mdVerification
The project's own test suite was run before and after this change; it introduces no new test failures or lint violations.