feat: per-session working directory for shell + coder - #327
Merged
Conversation
…tadata Add a per-session workingDir to Conversation, round-trip it through session metadata (metadataFor / conversationFromMeta), seed new chats from a last-used localStorage default, and add load/saveLastWorkingDir helpers. Foundation for the directory picker and the harness base_dir injection.
Add an optional `base_dir` field to every file-operation request (create-file, read-file, update-file, delete-file, move, list-folder, tree, search). When present, relative paths anchor at base_dir instead of the primary allowed root, absolute paths must canonicalize inside base_dir, and base_dir itself must canonicalize inside one of the worker's existing configured roots. The behavior is layered on top of the existing path resolver as a containment check plus relative-anchor, reusing the symlink-safe canonicalization and containing-root helpers — the jail core is untouched. When base_dir is absent the resolution path is unchanged. Introduce a distinct C218 error for the case where a path is inside an allowed root but outside the session directory; it names the session dir rather than reusing the generic "outside every allowed root" wording, so the rejection does not contradict coder::info's allowed-roots list. A base_dir that escapes every root stays C215. Regenerate the wire-schema goldens for the new optional field and add resolver unit tests for relative anchoring, absolute containment, the C218 DX rejection, base_dir-outside-roots, dotdot/symlink escapes, and a back-compat test proving base_dir=None resolves identically to the existing path.
Add an optional per-call base_dir field to shell::exec/exec_bg and every shell::fs::* request. When set, the call is scoped to that session directory: relative paths anchor at base_dir instead of the global host_root, and absolute paths must canonicalize inside base_dir. base_dir itself must canonicalize inside the configured host_root, else the call is rejected. For exec, base_dir becomes both the confinement root for cwd and the effective working directory when no cwd is given. For fs ops it scopes path validation and the lexical operand (rm/mv/chmod/sed) so the validated and operated-on paths cannot diverge. An absolute path that is inside host_root but outside base_dir is rejected with a new distinct S220 code whose message names the session directory and directs the caller to a path under it, instead of the generic escapes-host_root wording that would contradict the worker's own roots. base_dir is layered on top of the existing jail resolver, reusing its canonicalize/containment helpers without touching the core algorithm. When base_dir is absent the behaviour is unchanged.
When a turn carries a working_dir in its options metadata, stamp it onto every outbound shell::* and coder::* call as the per-call base_dir field before invocation. The harness owns workspace scoping, so any model-supplied base_dir is overwritten and cannot widen the scope. A new pure workspace_inject::inject helper performs the stamp immutably, returning the args unchanged when no working_dir is set, when the call is not shell/coder, or when the args are not a JSON object. The system prompt also gains a 'Your working directory is <dir>.' aid line so the model reasons about relative paths sensibly.
Add a DirectoryPicker in the composer footer that browses the operator's coder roots (coder::info) one level at a time (coder::list-folder), with a self-documenting empty-state when no project roots are configured. The chosen dir is locked after the first send. Block the first send until a dir is chosen, and forward working_dir on every send via harness::send options.metadata so the harness scopes the turn's shell/coder calls (base_dir) — session metadata alone is not visible on the turn record.
Extract buildTurnMetadata and pin that working_dir is present iff a directory is set — the C1 regression guard (a dropped working_dir silently re-collides two sessions).
Open the directory picker to a list of remembered projects (recent-first, × to forget) instead of the raw root. Add a search box that filters the current level live and accepts a pasted absolute path to jump-to/select. Browsing is now an explicit 'browse to add a project' action. Recent projects persist in localStorage and are recorded on every selection.
…lent default) A new chat no longer silently inherits the last-used dir — it starts with no working directory and an explicit 'choose one before sending' state, so a chat never operates in a directory the user didn't pick. A pasted or remembered dir is validated against the live worker roots before it's accepted (stale/missing dirs are rejected with a clear message). The active dir is shown as a full-path banner above the composer, and it can be re-scoped mid-conversation, which drops a visible 'working directory changed' marker in the transcript.
- validate by selecting the CANONICAL dir coder echoes back (a file path or out-of-root path is rejected, not silently accepted as base_dir) - errMsg: surface the innermost handler message and tolerate escaped quotes (no more '[object Object]') - banner: themed folder icon + leaf-visible (rtl) truncation so deep paths stay distinguishable in the dock - clear a stale browse error when returning to the projects view - remove now-dead loadLastWorkingDir/saveLastWorkingDir + key, and correct the 'locks after first send' docs (re-scope is allowed)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 37 minutes and 28 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds per-call working-directory scoping across coder and shell path resolution, threads ChangesWorkspace-scoped working directory flow
Sequence Diagram(s)sequenceDiagram
participant ChatView
participant useConversations
participant realStream
participant turn_loop
participant workspace_inject
participant HostFsBackend
participant PathResolver
ChatView->>useConversations: setWorkingDir(id, dir)
ChatView->>realStream: backend.stream(..., workingDir)
realStream->>turn_loop: options.metadata.working_dir
turn_loop->>workspace_inject: inject(function_id, args, working_dir)
workspace_inject->>HostFsBackend: base_dir on shell::* args
workspace_inject->>PathResolver: base_dir on coder::* args
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
skill-check — worker0 verified, 27 skipped (no docs/).
Four for four. Nicely done. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shell/src/fs/sandbox.rs (1)
68-70: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winForward
base_dirto the sandbox filesystem call.
LsArgs.base_diris ignored here, so a harness-injected session scope is silently lost for sandbox targets. Propagate it when present, and apply the same pattern to the other sandbox fs dispatchers.Proposed fix for `ls`
- self.dispatch("sandbox::fs::ls", json!({ "path": req.path })) + let mut payload = json!({ "path": req.path }); + if let Some(base_dir) = req.base_dir { + payload["base_dir"] = json!(base_dir); + } + self.dispatch("sandbox::fs::ls", payload) .await🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shell/src/fs/sandbox.rs` around lines 68 - 70, The sandbox filesystem dispatchers are dropping the session scope because LsArgs.base_dir is not forwarded into the request payload. Update ls in sandbox::fs::Sandbox to include base_dir when present, and apply the same forwarding pattern to the other sandbox fs methods that build dispatch JSON so harness-injected scope is preserved consistently.
🧹 Nitpick comments (6)
coder/src/functions/read_file.rs (1)
400-422: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd handler-level
base_dircoverage for both read modes.This now changes two separate paths (
single_readandbatch_read), but the file doesn't add a test that a relative path resolves underbase_diror that an in-root escape returnsC218. The resolver tests won't catch drift in the single-response vs per-entry batch wire shapes here.Also applies to: 507-507, 775-812
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coder/src/functions/read_file.rs` around lines 400 - 422, Add handler-level coverage for `read_file` so both `single_read` and `batch_read` are verified with `base_dir` handling. Create a test that a relative path resolves under `req.base_dir` in the single-response path, and another that an in-root escape is rejected with `C218` in the batch path; place the coverage near `single_read`, `batch_read`, and the `ReadFileOutput` mapping so wire-shape differences are exercised.coder/README.md (1)
154-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument how callers opt into session scoping.
The new
C218row explains the failure mode, but the README still doesn't explain the newbase_dirrequest field anywhere in the function docs. Right now readers learn how session scoping fails before they learn how to use it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coder/README.md` at line 154, The README’s function docs do not explain the new `base_dir` request field before the `C218` error row, so update the caller-facing docs to describe how to opt into session scoping. Add a concise explanation near the main request/schema documentation that references the relevant request/type name used by the API, clarifies that `base_dir` enables session-scoped paths, and shows how it interacts with the session directory so readers see usage before failure cases.coder/src/functions/search.rs (1)
197-197: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin
base_dirbehavior in the search tests.All of the updated fixtures still hardcode
base_dir: None, so this new resolver branch has no search-level coverage for scoped walks orC218escapes. A small positive case plus one escape case would keep the handler-specific path/output behavior from drifting.Also applies to: 537-947
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coder/src/functions/search.rs` at line 197, The new resolver branch in search should be covered by tests, since the current fixtures still use base_dir: None and do not exercise scoped walks or C218 escape handling. Update the search-level tests around search.rs and the handler path/output behavior to add one positive case with a pinned base_dir and one escape case that verifies the resolver rejects or handles an out-of-scope path. Use the existing search handler flow around resolver.resolve_opt and the walk_root behavior to locate where to extend coverage.coder/src/functions/move_file.rs (1)
123-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a handler-level test for
base_dir: Some(...).The resolver has scoped tests, but this handler wiring should also pin that both
fromandtoresolve under the session directory. A small test with a root-levela.txtand a session-levela.txtwould catch regressions where either endpoint accidentally falls back to the primary root.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coder/src/functions/move_file.rs` around lines 123 - 155, Add a handler-level test for move_file::move_one/move_file that covers req.base_dir being Some(...), and verify both spec.from and spec.to resolve within the session-scoped directory rather than falling back to the primary root. Use a setup with a root-level a.txt and a session-level a.txt so the test pins the PathResolver::require_writable_opt behavior through the handler wiring and catches regressions in endpoint resolution.coder/src/functions/update_file.rs (1)
1902-3244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a scoped update-file regression.
All updated handler tests preserve legacy
Nonebehavior, but none verify that edits land underbase_diror that../escapes fail at the handler boundary. One focusedbase_dir: Some(...)test would cover this PR’s main contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coder/src/functions/update_file.rs` around lines 1902 - 3244, Add a scoped regression test for the handler’s base_dir behavior in update_file tests. Verify that `handle`/`UpdateFileInput` applies edits relative to `base_dir` when `base_dir: Some(...)` is set, and that a path using `../` is rejected at the handler boundary. Use the existing `setup`, `handle`, `UpdateFileInput`, and `UpdateFileSpec` symbols to add one focused end-to-end case that covers both the allowed in-scope edit and the escaped-path failure.coder/src/functions/create_file.rs (1)
226-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one
base_dir: Some(...)handler regression.The production branch added in Line 122 is only covered here with
base_dir: None. A small test that creates into a scoped subdirectory and rejects../escape.txtwould catch accidental regressions in the handler wiring, not just the resolver unit tests.Also applies to: 262-262, 286-286, 315-315, 341-341, 370-370, 402-402, 431-431
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coder/src/functions/create_file.rs` at line 226, Add a regression test for the create-file handler path that uses a non-null base_dir instead of only the existing base_dir: None cases. In the relevant create_file test setup, wire a scoped subdirectory via base_dir: Some(...) and verify that a request targeting ../escape.txt is rejected, so the handler wiring is covered alongside the resolver behavior. Use the existing create_file handler/test helpers in this module to keep the new case consistent with the current test structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@coder/src/functions/delete_file.rs`:
- Around line 74-92: Block deletion of the session working directory in
delete_one: a path like "." under a project subdir can resolve to the active
session directory and slip past PathResolver::is_root because that check only
covers configured roots. Add an explicit guard after
resolver.require_writable_opt(base_dir, rel) that rejects deleting the resolved
session/work directory itself (and keep the existing recursive/root
protections), so DeleteFileResult never allows removing the active project
directory.
In `@coder/src/path/mod.rs`:
- Around line 334-339: The resolve_in method in path::mod currently allows
base_dir to be accepted via fallback canonicalization even when it does not
already exist, which can violate the working-directory contract. Update
resolve_in to require base_dir to canonicalize strictly as an existing directory
before calling containing_root or resolving the request path, and reject
non-directories or missing paths instead of relying on
canonicalize_wire(Path::new(base_dir)) fallback.
In `@coder/tests/golden/schemas/coder.move.json`:
- Around line 51-58: The top-level tool description still implies all paths
anchor only to the primary allowed root, which conflicts with the new `base_dir`
field. Update the main description in `coder.move.json` to explicitly mention
`base_dir` as the alternate anchoring mode, and make the wording consistent with
the `base_dir` property description so callers understand paths resolve against
the primary allowed root unless `base_dir` is set.
In `@coder/tests/golden/schemas/coder.read-file.json`:
- Around line 78-85: The top-level description for the read-file schema still
describes relative paths as resolving only against the primary allowed root, but
the new base_dir behavior in the base_dir property changes that for
session-scoped calls. Update the top-level description in the coder.read-file
schema to mirror the base_dir caveat, using the same terminology as the base_dir
field and the path/paths[] handling so the overall contract is consistent.
In `@coder/tests/golden/schemas/coder.search.json`:
- Around line 20-27: The top-level search schema description is stale and still
says path is relative only to the primary root, which conflicts with the new
base_dir behavior. Update the search schema documentation in the coder.search
JSON schema so it matches the base_dir semantics: the path can anchor relative
to base_dir when provided, while still staying within allowed roots. Use the
existing base_dir property description as the source of truth and adjust the
top-level search description accordingly.
In `@coder/tests/golden/schemas/coder.tree.json`:
- Around line 13-20: The top-level tool description is still describing path
resolution as if everything is relative to the primary allowed root, which does
not match the updated base_dir behavior. Update the description in
coder.tree.json so it mirrors the base_dir semantics already documented on the
property, including the session-scoped exception where a relative path can
anchor to base_dir and the resolved folder must remain inside it. Use the
existing base_dir field text and the top-level description entry as the places
to align.
In `@console/web/src/components/chat/ChatView.tsx`:
- Around line 208-219: The send guard in ChatView currently returns after
onSubmit has already caused Composer to clear the draft, so the user’s prompt is
lost when the real backend has no working directory. Update the submit flow
between Composer and ChatView so the failure is detected before clearing state,
or make onSubmit return a success/failure result that Composer uses to decide
whether to clear the editor and attachments. Use the ChatView handling around
backend.id, conversation.workingDir, and the Composer.tsx onSubmit path to keep
the draft intact when submission is blocked.
In `@console/web/src/components/chat/DirectoryPicker.tsx`:
- Around line 232-240: The jumpTo flow in DirectoryPicker allows an invalid or
out-of-root path to become selectable before folder validation succeeds. Update
jumpTo() so it does not store the requested path as the active selection until
loadFolder() completes successfully, and use the canonical path returned from
the successful folder load result (res.path) instead of the raw input. Also
adjust the header action logic that depends on path so “use this folder” is only
enabled after a verified folder load, and ensure select(path) can only receive a
validated directory from the browse state.
In `@console/web/src/hooks/use-conversations.ts`:
- Around line 110-118: The live metadata update path in use-conversations is
still dropping working_dir, so conversation state can become stale after
subscribeSessionDirectory(...).onMetaUpdated. Update the metadata merge/reducer
that handles md to also apply md.working_dir alongside the existing
model/mode/title updates, using the same Conversation/metadataFor flow so later
ChatView sends use the refreshed directory.
In `@console/web/src/stories/playground/harness.tsx`:
- Line 159: The playground harness is passing a no-op working directory updater,
which prevents real backend scenarios from setting conversation.workingDir and
unblocking the first send in ChatView. Update the onUpdateWorkingDir handler in
the harness to actually persist the selected working directory into the convo
state used by the playground scenario, so ChatView can observe the change and
proceed normally.
In `@harness/src/turn_loop.rs`:
- Around line 468-475: The working_dir metadata is being used too directly, so
empty strings are treated as real values and raw path text can be injected into
prompts. Centralize the working_dir extraction used by turn_loop.rs and
workspace_inject::inject so it returns None for empty or blank values, matching
the metadata contract. Then update with_working_dir_aid() to render the
directory via an escaped/sanitized representation instead of interpolating the
raw path, and ensure both call sites use the shared normalized value.
In `@shell/src/exec/policy.rs`:
- Around line 299-321: `build_overrides` is now promoting `base_dir` into
`ExecOverrides.cwd`, which causes sandbox-targeted `shell::*` execs to be
treated as host overrides and rejected. Update
`build_overrides`/`confine_base_dir` so `base_dir` stays separate from the host
cwd path until the host execution branch, or adjust the harness injection so
`working_dir` is not added for sandbox targets; use the existing
`build_overrides`, `ExecOverrides.cwd`, and `confine_cwd`/`confine_base_dir`
flow to keep scoped chats reaching the VM.
In `@shell/src/fs/host.rs`:
- Around line 417-424: Adjust the path-denial handling in
host::fs::host::canonicalize_with_fallback / the S215 branch so relative paths
that resolve inside host_root but outside the session base are classified as
S220 instead of falling through as S215. Extend the check around the existing
Path::new(path).is_absolute() logic to also evaluate relative inputs after
canonicalization, then return the session-scoped error code when canon is within
host_root and not within base. Keep the existing denylist and host_root checks
intact, but ensure the path classification uses the resolved canonical path
rather than the original absolute/relative form.
In `@shell/src/functions/exec_bg.rs`:
- Around line 47-53: The sandbox rejection path in exec_bg should account for
base_dir, since build_overrides now treats it as a populated override and the
current message only tells callers to remove cwd/env/stdin. Update the error
handling around build_overrides and the associated rejection message in exec_bg
to explicitly mention base_dir as unsupported here, or reject it directly before
returning, so callers get corrected toward the actual offending field.
---
Outside diff comments:
In `@shell/src/fs/sandbox.rs`:
- Around line 68-70: The sandbox filesystem dispatchers are dropping the session
scope because LsArgs.base_dir is not forwarded into the request payload. Update
ls in sandbox::fs::Sandbox to include base_dir when present, and apply the same
forwarding pattern to the other sandbox fs methods that build dispatch JSON so
harness-injected scope is preserved consistently.
---
Nitpick comments:
In `@coder/README.md`:
- Line 154: The README’s function docs do not explain the new `base_dir` request
field before the `C218` error row, so update the caller-facing docs to describe
how to opt into session scoping. Add a concise explanation near the main
request/schema documentation that references the relevant request/type name used
by the API, clarifies that `base_dir` enables session-scoped paths, and shows
how it interacts with the session directory so readers see usage before failure
cases.
In `@coder/src/functions/create_file.rs`:
- Line 226: Add a regression test for the create-file handler path that uses a
non-null base_dir instead of only the existing base_dir: None cases. In the
relevant create_file test setup, wire a scoped subdirectory via base_dir:
Some(...) and verify that a request targeting ../escape.txt is rejected, so the
handler wiring is covered alongside the resolver behavior. Use the existing
create_file handler/test helpers in this module to keep the new case consistent
with the current test structure.
In `@coder/src/functions/move_file.rs`:
- Around line 123-155: Add a handler-level test for
move_file::move_one/move_file that covers req.base_dir being Some(...), and
verify both spec.from and spec.to resolve within the session-scoped directory
rather than falling back to the primary root. Use a setup with a root-level
a.txt and a session-level a.txt so the test pins the
PathResolver::require_writable_opt behavior through the handler wiring and
catches regressions in endpoint resolution.
In `@coder/src/functions/read_file.rs`:
- Around line 400-422: Add handler-level coverage for `read_file` so both
`single_read` and `batch_read` are verified with `base_dir` handling. Create a
test that a relative path resolves under `req.base_dir` in the single-response
path, and another that an in-root escape is rejected with `C218` in the batch
path; place the coverage near `single_read`, `batch_read`, and the
`ReadFileOutput` mapping so wire-shape differences are exercised.
In `@coder/src/functions/search.rs`:
- Line 197: The new resolver branch in search should be covered by tests, since
the current fixtures still use base_dir: None and do not exercise scoped walks
or C218 escape handling. Update the search-level tests around search.rs and the
handler path/output behavior to add one positive case with a pinned base_dir and
one escape case that verifies the resolver rejects or handles an out-of-scope
path. Use the existing search handler flow around resolver.resolve_opt and the
walk_root behavior to locate where to extend coverage.
In `@coder/src/functions/update_file.rs`:
- Around line 1902-3244: Add a scoped regression test for the handler’s base_dir
behavior in update_file tests. Verify that `handle`/`UpdateFileInput` applies
edits relative to `base_dir` when `base_dir: Some(...)` is set, and that a path
using `../` is rejected at the handler boundary. Use the existing `setup`,
`handle`, `UpdateFileInput`, and `UpdateFileSpec` symbols to add one focused
end-to-end case that covers both the allowed in-scope edit and the escaped-path
failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ca5c8f0-098b-471d-9562-a8dc3055173a
⛔ Files ignored due to path filters (1)
coder/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (46)
coder/README.mdcoder/src/error.rscoder/src/functions/create_file.rscoder/src/functions/delete_file.rscoder/src/functions/list_folder.rscoder/src/functions/move_file.rscoder/src/functions/read_file.rscoder/src/functions/search.rscoder/src/functions/tree.rscoder/src/functions/update_file.rscoder/src/path/mod.rscoder/tests/golden/schemas/coder.create-file.jsoncoder/tests/golden/schemas/coder.delete-file.jsoncoder/tests/golden/schemas/coder.list-folder.jsoncoder/tests/golden/schemas/coder.move.jsoncoder/tests/golden/schemas/coder.read-file.jsoncoder/tests/golden/schemas/coder.search.jsoncoder/tests/golden/schemas/coder.tree.jsoncoder/tests/golden/schemas/coder.update-file.jsoncoder/tests/golden_errors.rscoder/tests/update_ops.rsconsole/web/src/components/chat/ChatPanel.tsxconsole/web/src/components/chat/ChatView.tsxconsole/web/src/components/chat/Composer.tsxconsole/web/src/components/chat/DirectoryPicker.tsxconsole/web/src/hooks/use-conversations.tsconsole/web/src/lib/backend/real-metadata.test.tsconsole/web/src/lib/backend/real.tsconsole/web/src/lib/backend/types.tsconsole/web/src/lib/storage.tsconsole/web/src/stories/playground/harness.tsxconsole/web/src/types/chat.tsharness/src/lib.rsharness/src/turn_loop.rsharness/src/workspace_inject.rsshell/src/exec/host.rsshell/src/exec/policy.rsshell/src/fs/host.rsshell/src/fs/mod.rsshell/src/fs/sandbox.rsshell/src/functions/exec.rsshell/src/functions/exec_bg.rsshell/src/functions/types.rsshell/src/scode.rsshell/tests/host_fs_branches.rsshell/tests/sandbox_dispatch.rs
Comment on lines
+334
to
+339
| pub fn resolve_in(&self, base_dir: &str, path: &str) -> Result<PathBuf, CoderError> { | ||
| // (c) base_dir must canonicalise inside an EXISTING allowed root. | ||
| // Reuse the shared canonicalisation so a `..`/symlink escape in the | ||
| // session dir itself fails closed exactly like a wire path would. | ||
| let base_canon = self.canonicalize_wire(base_dir, Path::new(base_dir))?; | ||
| if self.containing_root(&base_canon).is_none() { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require base_dir to be an existing directory before scoping paths.
Line 338 uses fallback canonicalization, so a missing base_dir under an allowed root can still be accepted and later created by mutating calls. That breaks the “working directory” contract; canonicalize base_dir strictly and reject non-directories before resolving request paths.
Suggested direction
- let base_canon = self.canonicalize_wire(base_dir, Path::new(base_dir))?;
+ let base_canon = std::fs::canonicalize(base_dir).map_err(|e| {
+ if e.kind() == std::io::ErrorKind::InvalidInput
+ || e.kind() == std::io::ErrorKind::NotFound
+ {
+ CoderError::not_found_or_denied(base_dir)
+ } else {
+ CoderError::Io(format!("canonicalize base_dir {base_dir}: {e}"))
+ }
+ })?;
+ if !base_canon.is_dir() {
+ return Err(CoderError::BadInput(format!(
+ "base_dir is not a directory: {base_dir}"
+ )));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn resolve_in(&self, base_dir: &str, path: &str) -> Result<PathBuf, CoderError> { | |
| // (c) base_dir must canonicalise inside an EXISTING allowed root. | |
| // Reuse the shared canonicalisation so a `..`/symlink escape in the | |
| // session dir itself fails closed exactly like a wire path would. | |
| let base_canon = self.canonicalize_wire(base_dir, Path::new(base_dir))?; | |
| if self.containing_root(&base_canon).is_none() { | |
| pub fn resolve_in(&self, base_dir: &str, path: &str) -> Result<PathBuf, CoderError> { | |
| // (c) base_dir must canonicalise inside an EXISTING allowed root. | |
| // Reuse the shared canonicalisation so a `..`/symlink escape in the | |
| // session dir itself fails closed exactly like a wire path would. | |
| let base_canon = std::fs::canonicalize(base_dir).map_err(|e| { | |
| if e.kind() == std::io::ErrorKind::InvalidInput | |
| || e.kind() == std::io::ErrorKind::NotFound | |
| { | |
| CoderError::not_found_or_denied(base_dir) | |
| } else { | |
| CoderError::Io(format!("canonicalize base_dir {base_dir}: {e}")) | |
| } | |
| })?; | |
| if !base_canon.is_dir() { | |
| return Err(CoderError::BadInput(format!( | |
| "base_dir is not a directory: {base_dir}" | |
| ))); | |
| } | |
| if self.containing_root(&base_canon).is_none() { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@coder/src/path/mod.rs` around lines 334 - 339, The resolve_in method in
path::mod currently allows base_dir to be accepted via fallback canonicalization
even when it does not already exist, which can violate the working-directory
contract. Update resolve_in to require base_dir to canonicalize strictly as an
existing directory before calling containing_root or resolving the request path,
and reject non-directories or missing paths instead of relying on
canonicalize_wire(Path::new(base_dir)) fallback.
Comment on lines
+51
to
+58
| "base_dir": { | ||
| "default": null, | ||
| "description": "Optional per-call session working directory. When set, relative `from`/`to` paths anchor here instead of the primary allowed root, and BOTH resolved endpoints must stay inside it. `base_dir` itself must canonicalize inside an allowed root (`coder::info` lists them). Omit to resolve against the primary allowed root exactly as before.", | ||
| "type": [ | ||
| "string", | ||
| "null" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the top-level description to mention base_dir.
The new property is documented here, but the tool description still describes only primary-root anchoring. Add “unless base_dir is set” there too so callers don’t see conflicting path semantics.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@coder/tests/golden/schemas/coder.move.json` around lines 51 - 58, The
top-level tool description still implies all paths anchor only to the primary
allowed root, which conflicts with the new `base_dir` field. Update the main
description in `coder.move.json` to explicitly mention `base_dir` as the
alternate anchoring mode, and make the wording consistent with the `base_dir`
property description so callers understand paths resolve against the primary
allowed root unless `base_dir` is set.
Comment on lines
+78
to
+85
| "base_dir": { | ||
| "default": null, | ||
| "description": "Optional per-call session working directory. When set, every relative path (the single `path` or each `paths[]` entry, in both the bare string and `{path,...}` object forms) anchors here instead of the primary allowed root, and every resolved path must stay inside it. `base_dir` itself must canonicalize inside an allowed root (`coder::info` lists them). Omit to resolve against the primary allowed root exactly as before.", | ||
| "type": [ | ||
| "string", | ||
| "null" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the top-level description to include session anchoring.
The added base_dir property changes how path/paths[] resolve, but the top-level description still says paths are relative to the primary allowed root. Please mirror the base_dir caveat there as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@coder/tests/golden/schemas/coder.read-file.json` around lines 78 - 85, The
top-level description for the read-file schema still describes relative paths as
resolving only against the primary allowed root, but the new base_dir behavior
in the base_dir property changes that for session-scoped calls. Update the
top-level description in the coder.read-file schema to mirror the base_dir
caveat, using the same terminology as the base_dir field and the path/paths[]
handling so the overall contract is consistent.
Comment on lines
+20
to
+27
| "base_dir": { | ||
| "default": null, | ||
| "description": "Optional per-call session working directory. When set, a relative `path` anchors here instead of the primary allowed root, and the walk root must stay inside it. `base_dir` itself must canonicalize inside an allowed root (`coder::info` lists them). Result paths stay absolute. Omit to resolve against the primary allowed root exactly as before.", | ||
| "type": [ | ||
| "string", | ||
| "null" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the schema description consistent with base_dir.
The top-level search description still says path is relative to the primary root. Since this property now allows session-relative anchoring, update that description to avoid stale tool guidance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@coder/tests/golden/schemas/coder.search.json` around lines 20 - 27, The
top-level search schema description is stale and still says path is relative
only to the primary root, which conflicts with the new base_dir behavior. Update
the search schema documentation in the coder.search JSON schema so it matches
the base_dir semantics: the path can anchor relative to base_dir when provided,
while still staying within allowed roots. Use the existing base_dir property
description as the source of truth and adjust the top-level search description
accordingly.
| modelOptions={PLAYGROUND_MODEL_OPTIONS} | ||
| onUpdateModel={setModel} | ||
| onUpdateMode={setMode} | ||
| onUpdateWorkingDir={() => {}} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't use a no-op working-directory updater here.
If a playground scenario uses backend.id === 'real', console/web/src/components/chat/ChatView.tsx Lines 208-218 blocks the first send until conversation.workingDir is set. This callback never updates convo, so the picker cannot unblock the chat.
Suggested fix
+ const setWorkingDir = useCallback((_id: string, workingDir: string) => {
+ setConvo((c) => ({ ...c, workingDir, updatedAt: Date.now() }))
+ }, [])
+
return (
@@
<ChatView
key={convo.id}
conversation={convo}
backend={tappedBackend}
modelOptions={PLAYGROUND_MODEL_OPTIONS}
onUpdateModel={setModel}
onUpdateMode={setMode}
- onUpdateWorkingDir={() => {}}
+ onUpdateWorkingDir={setWorkingDir}
onAppendMessage={appendMessage}
onPatchMessage={updateMessage}
onCompactConversation={compactConversation}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onUpdateWorkingDir={() => {}} | |
| const setWorkingDir = useCallback((_id: string, workingDir: string) => { | |
| setConvo((c) => ({ ...c, workingDir, updatedAt: Date.now() })) | |
| }, []) | |
| onUpdateWorkingDir={setWorkingDir} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@console/web/src/stories/playground/harness.tsx` at line 159, The playground
harness is passing a no-op working directory updater, which prevents real
backend scenarios from setting conversation.workingDir and unblocking the first
send in ChatView. Update the onUpdateWorkingDir handler in the harness to
actually persist the selected working directory into the convo state used by the
playground scenario, so ChatView can observe the change and proceed normally.
Comment on lines
+468
to
+475
| let working_dir = record | ||
| .options | ||
| .metadata | ||
| .as_ref() | ||
| .and_then(|m| m.get("working_dir")) | ||
| .and_then(Value::as_str); | ||
| let scoped_args = | ||
| crate::workspace_inject::inject(&call.function_id, eff_args, working_dir); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Normalize and escape working_dir before using it.
record.options.metadata["working_dir"] is trusted verbatim in both the call injector and with_working_dir_aid(). That means "" currently becomes base_dir: "", and a directory name containing a newline is injected into the system prompt as extra system-level text. Please centralize extraction so only non-empty values are treated as present, and render the prompt aid with an escaped representation instead of interpolating the path raw.
🔒 Suggested hardening
+fn working_dir(record: &TurnRecord) -> Option<&str> {
+ record
+ .options
+ .metadata
+ .as_ref()
+ .and_then(|m| m.get("working_dir"))
+ .and_then(Value::as_str)
+ .filter(|dir| !dir.is_empty())
+}
+
...
- let working_dir = record
- .options
- .metadata
- .as_ref()
- .and_then(|m| m.get("working_dir"))
- .and_then(Value::as_str);
+ let working_dir = working_dir(&record);
let scoped_args =
crate::workspace_inject::inject(&call.function_id, eff_args, working_dir);
...
- let working_dir = record
- .options
- .metadata
- .as_ref()
- .and_then(|m| m.get("working_dir"))
- .and_then(Value::as_str);
+ let working_dir = working_dir(record);
let Some(dir) = working_dir else {
return system_prompt;
};
- let line = format!("Your working directory is {dir}.");
+ let line = format!(
+ "Your working directory is {}.",
+ serde_json::to_string(dir).unwrap_or_else(|_| "\"<invalid>\"".into())
+ );Based on the metadata contract covered in console/web/src/lib/backend/real-metadata.test.ts, empty working_dir is supposed to be treated as absent.
Also applies to: 1032-1045
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@harness/src/turn_loop.rs` around lines 468 - 475, The working_dir metadata is
being used too directly, so empty strings are treated as real values and raw
path text can be injected into prompts. Centralize the working_dir extraction
used by turn_loop.rs and workspace_inject::inject so it returns None for empty
or blank values, matching the metadata contract. Then update
with_working_dir_aid() to render the directory via an escaped/sanitized
representation instead of interpolating the raw path, and ensure both call sites
use the shared normalized value.
Comment on lines
+417
to
+424
| if e.code == "S215" && Path::new(path).is_absolute() { | ||
| if let Ok(canon) = canonicalize_with_fallback(Path::new(path)) { | ||
| let inside_host_root = host_root_canon | ||
| .map(|hr| canon.starts_with(hr)) | ||
| .unwrap_or(false); | ||
| let denied = denylist_canon.iter().any(|d| canon.starts_with(d)); | ||
| if inside_host_root && !canon.starts_with(base) && !denied { | ||
| return Err(FsError::new( |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return S220 for relative escapes that stay inside host_root.
Line 417 only refines absolute paths. A relative path like ../secret.txt from base_dir=session resolves inside host_root but outside the session directory, so it currently falls through as S215 instead of the session-scoped S220 contract.
Proposed fix
- if e.code == "S215" && Path::new(path).is_absolute() {
- if let Ok(canon) = canonicalize_with_fallback(Path::new(path)) {
+ if e.code == "S215" {
+ let raw = Path::new(path);
+ let joined;
+ let candidate = if raw.is_absolute() {
+ raw
+ } else {
+ joined = base.join(raw);
+ joined.as_path()
+ };
+ if let Ok(canon) = canonicalize_with_fallback(candidate) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if e.code == "S215" && Path::new(path).is_absolute() { | |
| if let Ok(canon) = canonicalize_with_fallback(Path::new(path)) { | |
| let inside_host_root = host_root_canon | |
| .map(|hr| canon.starts_with(hr)) | |
| .unwrap_or(false); | |
| let denied = denylist_canon.iter().any(|d| canon.starts_with(d)); | |
| if inside_host_root && !canon.starts_with(base) && !denied { | |
| return Err(FsError::new( | |
| if e.code == "S215" { | |
| let raw = Path::new(path); | |
| let joined; | |
| let candidate = if raw.is_absolute() { | |
| raw | |
| } else { | |
| joined = base.join(raw); | |
| joined.as_path() | |
| }; | |
| if let Ok(canon) = canonicalize_with_fallback(candidate) { | |
| let inside_host_root = host_root_canon | |
| .map(|hr| canon.starts_with(hr)) | |
| .unwrap_or(false); | |
| let denied = denylist_canon.iter().any(|d| canon.starts_with(d)); | |
| if inside_host_root && !canon.starts_with(base) && !denied { | |
| return Err(FsError::new( |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shell/src/fs/host.rs` around lines 417 - 424, Adjust the path-denial handling
in host::fs::host::canonicalize_with_fallback / the S215 branch so relative
paths that resolve inside host_root but outside the session base are classified
as S220 instead of falling through as S215. Extend the check around the existing
Path::new(path).is_absolute() logic to also evaluate relative inputs after
canonicalization, then return the session-scoped error code when canon is within
host_root and not within base. Keep the existing denylist and host_root checks
intact, but ensure the path classification uses the resolved canonical path
rather than the original absolute/relative form.
sergiofilhowz
approved these changes
Jun 26, 2026
Resolve shell/src/functions/exec.rs: keep the per-call base_dir argument to build_overrides (this branch) while adopting main's iii-sdk 0.20 error path (iii_sdk::errors::Error::from). Brings in the iii-sdk 0.20.0 bump and the sandbox dispatch rename across shell.
rustfmt collapses the multi-line fn signatures and struct literals introduced with the per-call base_dir parameter (resolve_opt, require_writable_opt, move_one, CreateFileInput/UpdateFileInput). Greens 'coder: rust lint + test' (cargo fmt --check).
The merge left shell/Cargo.lock at 0.5.2 and harness/Cargo.lock at 1.0.3 while the merged Cargo.toml files carry main's bumps (0.5.5 / 1.0.4). Regenerate the lock entries so the lockfile matches the manifests.
A session-scoped delete (`base_dir` set) resolves `paths: ["."]` — or any path that canonicalizes back to base_dir — to the session directory, which is a SUBDIR of an allowed root. The existing `is_root` guard only covers the configured roots, so a recursive delete would wipe the active project dir. Add `PathResolver::session_root` (canonical base_dir via the same canonicalisation as resolve_in) and refuse the delete with C210 when the resolved target equals the session root. base_dir=None is unchanged. Tests cover deletion via "." and via an absolute path, plus a file inside the session dir still deleting (guard is not over-broad).
The harness stamps the session working directory onto every `shell::*` call as `base_dir`. `build_overrides` folds base_dir into `ExecOverrides.cwd`, so a sandbox-targeted exec from a scoped chat arrives with a populated host cwd override — which both exec sandbox paths reject as host-only (S210). Net effect: sandbox exec was broken for every working-dir-scoped conversation. base_dir scopes a HOST working directory; the sandbox runs against the VM's own filesystem, so an injected base_dir is meaningless there. Add `base_dir_for_target` and drop base_dir for sandbox targets before build_overrides. Genuine user-supplied cwd/env/stdin on a sandbox call are still rejected; only the harness base_dir is dropped.
ytallo
added a commit
that referenced
this pull request
Jun 28, 2026
Fold the standalone coder worker's file surface into shell: read, window, search, tree, list-folder, create, update, move, delete, and info now run inside shell against a single multi-root jail driven by `fs.host_roots` (the operator sets the root once; any `code.base_paths` is ignored). - offload the code read/scan handlers to spawn_blocking - honor the unified protected-path globs in `shell::fs` - fold the legacy `coder` config into the `code` block with a one-shot, never-widen migration marker (`migrated_from_coder`) - retire the standalone coder worker (remove its crate, release tooling, and console workers entry) and retarget the agent prompts/docs to shell - migrate the folded surface to iii-sdk 0.20 Preserves #327's per-session working-directory protections (the session dir cannot be deleted via a scoped `delete path="."`).
ytallo
added a commit
that referenced
this pull request
Jun 29, 2026
…fig) (#340) * feat(shell): fold the coder file surface into shell over a unified jail Fold the standalone coder worker's file surface into shell: read, window, search, tree, list-folder, create, update, move, delete, and info now run inside shell against a single multi-root jail driven by `fs.host_roots` (the operator sets the root once; any `code.base_paths` is ignored). - offload the code read/scan handlers to spawn_blocking - honor the unified protected-path globs in `shell::fs` - fold the legacy `coder` config into the `code` block with a one-shot, never-widen migration marker (`migrated_from_coder`) - retire the standalone coder worker (remove its crate, release tooling, and console workers entry) and retarget the agent prompts/docs to shell - migrate the folded surface to iii-sdk 0.20 Preserves #327's per-session working-directory protections (the session dir cannot be deleted via a scoped `delete path="."`). * test: harden shell coder merge coverage * Add adversarial coder BDD coverage * feat(console): gate the chat working-directory picker on shell-worker presence The working-directory picker browses via the shell-served `coder::*` functions and scopes a chat to a directory the shell worker enforces, so it only makes sense when shell is connected. Show the picker and its status banner only when the shell worker is present, and drop the requirement to choose a directory before sending — chats fall back to the default workspace. - Extract the approval-gate presence probe into a reusable `useWorkerPresence` hook: an initial `engine::workers::list` read plus a live `worker` add/remove lifecycle trigger. Refactor `useApprovalGateStatus` to wrap it and add `useShellStatus`. - Expose `shellAvailable` through the conversations context; `ChatView` gates the picker and banner on `backend.id === 'real' && shellAvailable`. - Register the shell worker with `WorkerMetadata { name: "shell" }` so it is discoverable in `engine::workers::list`; it otherwise connects unnamed and the console can't detect it.
This was referenced Jun 30, 2026
This was referenced Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Lets each chat in the console pick a project directory, and confines that chat's shell and coder file operations to it — so two chats running at once no longer step on each other's files.
Workers — per-call
base_dirbase_dir. When set, relative paths anchor at it and absolute paths must stay inside it; an escape is rejected with a message that names the session directory (C218coder /S220shell). Whenbase_diris absent, behavior is byte-for-byte unchanged (back-compat).MIRROR-INVARIANTcanonicalize/normalize) is not modified —base_diris a containment check layered on top of the existing resolver.Harness — injection
shell::*/coder::*call asbase_dir(overwriting any model-supplied value), and tells the model its working directory in the system prompt. The directory ridesharness::sendoptions.metadata.working_dir(the turn record), so it is visible at the dispatch choke point.Console — the picker
coder::info/coder::list-folder). Pasted or remembered paths are validated against the live roots before they're accepted.Test plan
cargo test -p coder/-p shell/-p harness— per-callbase_dir: relative anchors at it, absolute-outside-base_dirrejected,../symlink escapes still fail, andbase_dir-absent reproduces prior behavior.vitest—working_diris forwarded onharness::send(buildTurnMetadata).src/main.rs(relative) → land in their own dir, no collision; an absolute cross-dir write is rejected;pwdreports the session dir.Not in scope
coderworker intoshell(a separate follow-up PR).Summary by CodeRabbit
New Features
Bug Fixes
Documentation