feat(shell): add workspace picker control plane - #376
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
2 tasks
📝 WalkthroughWalkthroughAdds three new ChangesWorkspace picker and base_dir scoping
Sequence Diagram(s)sequenceDiagram
participant UI as DirectoryPicker
participant Harness
participant Shell as shell worker
participant workspace as functions::workspace
UI->>Harness: shell::workspace::roots
Note over Harness: is_scoped_function=false, pass through
Harness->>Shell: shell::workspace::roots
Shell->>workspace: workspace_roots(cfg)
workspace-->>UI: { roots: [...] }
UI->>Harness: shell::workspace::list { path }
Harness->>Shell: shell::workspace::list
Shell->>workspace: list_workspace_dirs(req, cfg)
workspace-->>UI: { entries: [{name, path, kind}] }
UI->>Harness: shell::workspace::validate { path }
Harness->>Shell: shell::workspace::validate
Shell->>workspace: validate_workspace_path(req, cfg)
workspace-->>UI: { path: canonical }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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, 26 skipped (no docs/).
Four for four. Nicely done. |
ytallo
force-pushed
the
fix/console-working-dir-control-plane
branch
from
June 30, 2026 02:28
feed202 to
2dd7fab
Compare
ytallo
marked this pull request as ready for review
June 30, 2026 02:46
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
console/web/src/components/chat/DirectoryPicker.tsx (1)
167-183: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the canonical
res.pathafter a browse jump.Line 259 keeps the raw pasted path even though
shell::workspace::listcanonicalizes it. Jumping to a symlink or/a/../bcan then list one directory while use this folder and go up act on another. Promoteres.pathintopath(and root matching) in the browse flow, the same wayvalidateAndSelect()already does for selection.Proposed fix
- const loadFolder = useCallback(async (target: string) => { + const loadFolder = useCallback(async (target: string): Promise<string | null> => { setLoading(true) setError(null) try { const client = await getIiiClient() const res = await client.trigger<WorkspaceListResult>( WORKSPACE_LIST_FUNCTION_ID, { path: target, page_size: 200, }, ) + const resolved = res?.path ?? target const names = (res?.entries ?? []) .filter((e) => e.kind === 'dir') .map((e) => e.path) .sort((a, b) => a.localeCompare(b)) setDirs(names) + return resolved } catch (err) { setError(errMsg(err)) setDirs([]) + return null } finally { setLoading(false) } }, []) @@ const jumpTo = useCallback( async (raw: string) => { const p = raw.trim().replace(/\/+$/, '') || '/' setView('browse') setQuery('') const r = await ensureRoots() - const matchedRoot = - [...r] - .sort((a, b) => b.length - a.length) - .find((x) => p === x || p.startsWith(`${x}/`)) ?? - r[0] ?? - null - setRoot(matchedRoot) - setPath(p) - await loadFolder(p) + const resolved = (await loadFolder(p)) ?? p + const matchedRoot = + [...r] + .sort((a, b) => b.length - a.length) + .find((x) => resolved === x || resolved.startsWith(`${x}/`)) ?? + r[0] ?? + null + setRoot(matchedRoot) + setPath(resolved) }, [ensureRoots, loadFolder], )Also applies to: 246-260
🤖 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/components/chat/DirectoryPicker.tsx` around lines 167 - 183, The browse flow in DirectoryPicker is keeping the original pasted target instead of the canonical path returned by shell::workspace::list, which can desync listing vs actions like use this folder and go up. Update loadFolder and the browse/jump handling to use res.path as the authoritative path after a successful list, and make root matching depend on that canonical path just as validateAndSelect() already does for selection.
🤖 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 `@shell/src/code/functions/mod.rs`:
- Around line 294-295: The handlers that call
resolver.session_scoped(req.base_dir.as_deref()) are trusting req.base_dir from
deserialized request payloads, which lets direct coder::* calls widen access
outside configured roots. Fix this by preventing public request structs from
accepting base_dir at all, or by stripping/overwriting it at the registration
boundary before any handler like the resolver/session_scoped paths use it. Keep
the harness-injected base_dir separate from the public request types so only
internal harness code can supply it, and direct invocations always use a safe,
server-controlled value.
In `@shell/src/code/path.rs`:
- Around line 384-395: resolve_in() currently allows a canonicalized regular
file to be used as base_dir, which should be rejected to match confine_base_dir
in shell/src/fs/host.rs and shell/src/exec/policy.rs. Update the resolve_in()
flow to verify the canonicalized base_path is an existing directory before
proceeding with containing_root and relative path joins, and return a
BadInput-style error when base_dir points to a file rather than a directory.
In `@shell/src/fs/host.rs`:
- Around line 420-428: The access_roots helper is dropping parent-directory
context when base_dir_canon is added alone, which can let protected scoped roots
bypass non_accessible_globs checks. Update access_roots so the selected base
directory’s parent is also included in the effective roots (or reject protected
selected roots outright), and verify the protected-path matching path in
shell/src/fs/host.rs still preserves names like .git for access checks.
---
Outside diff comments:
In `@console/web/src/components/chat/DirectoryPicker.tsx`:
- Around line 167-183: The browse flow in DirectoryPicker is keeping the
original pasted target instead of the canonical path returned by
shell::workspace::list, which can desync listing vs actions like use this folder
and go up. Update loadFolder and the browse/jump handling to use res.path as the
authoritative path after a successful list, and make root matching depend on
that canonical path just as validateAndSelect() already does for selection.
🪄 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: c789d191-9a28-401a-8e85-ce4e23b3cbcd
📒 Files selected for processing (32)
console/web/src/components/chat/DirectoryPicker.test.tsconsole/web/src/components/chat/DirectoryPicker.tsxconsole/web/src/hooks/use-shell-status.tsconsole/web/src/lib/backend/real-metadata.test.tsconsole/web/src/lib/backend/real.tsharness/src/workspace_inject.rsshell/src/code/functions/create_file.rsshell/src/code/functions/delete_file.rsshell/src/code/functions/list_folder.rsshell/src/code/functions/mod.rsshell/src/code/functions/move_file.rsshell/src/code/functions/read_file.rsshell/src/code/functions/search.rsshell/src/code/functions/tree.rsshell/src/code/functions/update_file.rsshell/src/code/path.rsshell/src/exec/host.rsshell/src/exec/policy.rsshell/src/fs/host.rsshell/src/fs/mod.rsshell/src/functions/mod.rsshell/src/functions/types.rsshell/src/functions/workspace.rsshell/src/main.rsshell/tests/golden/schemas/coder.create-file.jsonshell/tests/golden/schemas/coder.delete-file.jsonshell/tests/golden/schemas/coder.list-folder.jsonshell/tests/golden/schemas/coder.move.jsonshell/tests/golden/schemas/coder.read-file.jsonshell/tests/golden/schemas/coder.search.jsonshell/tests/golden/schemas/coder.tree.jsonshell/tests/golden/schemas/coder.update-file.json
💤 Files with no reviewable changes (8)
- shell/tests/golden/schemas/coder.read-file.json
- shell/tests/golden/schemas/coder.update-file.json
- shell/tests/golden/schemas/coder.list-folder.json
- shell/tests/golden/schemas/coder.tree.json
- shell/tests/golden/schemas/coder.create-file.json
- shell/tests/golden/schemas/coder.move.json
- shell/tests/golden/schemas/coder.search.json
- shell/tests/golden/schemas/coder.delete-file.json
Comment on lines
+294
to
295
| let resolver = resolver.session_scoped(req.base_dir.as_deref()); | ||
| let cfg = cfg.read().await.clone(); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Don't treat wire base_dir as trusted input.
All of these handlers now widen the resolver with req.base_dir, but the corresponding request types still deserialize that field from raw JSON; #[schemars(skip)] only hides it from published schema. A direct coder::* call can therefore smuggle base_dir in the payload and escape the configured coder roots to any absolute directory that session_scoped() accepts. Strip or overwrite base_dir at the registration boundary, or split public request types from the internal harness-injected ones. Based on review stack context, harness-side stripping only protects the harness path; it does not harden direct function invocations.
Also applies to: 312-313, 330-331, 348-349, 365-366, 382-383, 400-401, 414-416
🤖 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/code/functions/mod.rs` around lines 294 - 295, The handlers that
call resolver.session_scoped(req.base_dir.as_deref()) are trusting req.base_dir
from deserialized request payloads, which lets direct coder::* calls widen
access outside configured roots. Fix this by preventing public request structs
from accepting base_dir at all, or by stripping/overwriting it at the
registration boundary before any handler like the resolver/session_scoped paths
use it. Keep the harness-injected base_dir separate from the public request
types so only internal harness code can supply it, and direct invocations always
use a safe, server-controlled value.
Comment on lines
384
to
395
| pub fn resolve_in(&self, base_dir: &str, path: &str) -> Result<PathBuf, CoderError> { | ||
| let base_path = Path::new(base_dir); | ||
| if !base_path.is_absolute() { | ||
| return Err(CoderError::BadInput(format!( | ||
| "base_dir must be an absolute path: {base_dir}" | ||
| ))); | ||
| } | ||
| // (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))?; | ||
| let base_canon = self.canonicalize_wire(base_dir, base_path)?; | ||
| if self.containing_root(&base_canon).is_none() { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject file paths as base_dir here.
resolve_in() still accepts any canonicalized path under an allowed root, including regular files. That diverges from shell/src/fs/host.rs::confine_base_dir and shell/src/exec/policy.rs::confine_base_dir, both of which require an existing directory. With the current code, base_dir=/allowed/file.txt passes containment and relative joins then build impossible descendants under that file, so callers get a late I/O failure instead of a clean contract error.
Suggested fix
let base_canon = self.canonicalize_wire(base_dir, base_path)?;
+ 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() {
return Err(CoderError::OutsideBase(format!(
"base_dir is outside every allowed root: {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> { | |
| let base_path = Path::new(base_dir); | |
| if !base_path.is_absolute() { | |
| return Err(CoderError::BadInput(format!( | |
| "base_dir must be an absolute path: {base_dir}" | |
| ))); | |
| } | |
| // (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))?; | |
| let base_canon = self.canonicalize_wire(base_dir, base_path)?; | |
| if self.containing_root(&base_canon).is_none() { | |
| pub fn resolve_in(&self, base_dir: &str, path: &str) -> Result<PathBuf, CoderError> { | |
| let base_path = Path::new(base_dir); | |
| if !base_path.is_absolute() { | |
| return Err(CoderError::BadInput(format!( | |
| "base_dir must be an absolute path: {base_dir}" | |
| ))); | |
| } | |
| // (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, base_path)?; | |
| 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 `@shell/src/code/path.rs` around lines 384 - 395, resolve_in() currently allows
a canonicalized regular file to be used as base_dir, which should be rejected to
match confine_base_dir in shell/src/fs/host.rs and shell/src/exec/policy.rs.
Update the resolve_in() flow to verify the canonicalized base_path is an
existing directory before proceeding with containing_root and relative path
joins, and return a BadInput-style error when base_dir points to a file rather
than a directory.
Comment on lines
+420
to
+428
| fn access_roots(host_roots_canon: &[PathBuf], base_dir_canon: Option<&Path>) -> Vec<PathBuf> { | ||
| let mut roots = host_roots_canon.to_vec(); | ||
| if let Some(base) = base_dir_canon { | ||
| if !roots.iter().any(|r| r == base) { | ||
| roots.push(base.to_path_buf()); | ||
| } | ||
| } | ||
| roots | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Preserve parent-root context for protected glob checks.
Adding base_dir as the only extra root strips the selected directory name before matching. If the harness selects a protected directory like /repo/.git as base_dir, a child such as config is matched as config, so patterns like **/.git/** no longer block it. Include the selected directory’s parent in the effective roots, or reject protected selected roots, so scoped roots outside host_roots cannot bypass non_accessible_globs.
🛡️ Proposed fix
fn access_roots(host_roots_canon: &[PathBuf], base_dir_canon: Option<&Path>) -> Vec<PathBuf> {
let mut roots = host_roots_canon.to_vec();
if let Some(base) = base_dir_canon {
+ if let Some(parent) = base.parent() {
+ if !roots.iter().any(|r| r.as_path() == parent) {
+ roots.push(parent.to_path_buf());
+ }
+ }
if !roots.iter().any(|r| r == base) {
roots.push(base.to_path_buf());
}
}
roots
}📝 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
| fn access_roots(host_roots_canon: &[PathBuf], base_dir_canon: Option<&Path>) -> Vec<PathBuf> { | |
| let mut roots = host_roots_canon.to_vec(); | |
| if let Some(base) = base_dir_canon { | |
| if !roots.iter().any(|r| r == base) { | |
| roots.push(base.to_path_buf()); | |
| } | |
| } | |
| roots | |
| } | |
| fn access_roots(host_roots_canon: &[PathBuf], base_dir_canon: Option<&Path>) -> Vec<PathBuf> { | |
| let mut roots = host_roots_canon.to_vec(); | |
| if let Some(base) = base_dir_canon { | |
| if let Some(parent) = base.parent() { | |
| if !roots.iter().any(|r| r.as_path() == parent) { | |
| roots.push(parent.to_path_buf()); | |
| } | |
| } | |
| if !roots.iter().any(|r| r == base) { | |
| roots.push(base.to_path_buf()); | |
| } | |
| } | |
| roots | |
| } |
🤖 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 420 - 428, The access_roots helper is
dropping parent-directory context when base_dir_canon is added alone, which can
let protected scoped roots bypass non_accessible_globs checks. Update
access_roots so the selected base directory’s parent is also included in the
effective roots (or reject protected selected roots outright), and verify the
protected-path matching path in shell/src/fs/host.rs still preserves names like
.git for access checks.
andersonleal
approved these changes
Jun 30, 2026
sergiofilhowz
approved these changes
Jun 30, 2026
andersonleal
added a commit
that referenced
this pull request
Jun 30, 2026
* feat(workflow): durable DAG workflow worker A new `workflow` worker that orchestrates multi-agent pipelines as a durable directed-acyclic graph on the iii engine. - DAG core: typed node/run state, dependency validation, multi-input joins, topological scheduling, and crash-safe durable persistence with reconcile. - Execution: per-node (optionally nested) agent sessions, fast-wake on turn-completed, caller notify callbacks, and a sweep loop with timeouts. - Permissions: inherit-by-default node permissions, per-node router deny. - Fire-and-forget `workflow::start`: callers get the run_id back immediately and receive the outcome via `reply_to` / `notify` or by polling `workflow::status`; no blocking `await` path. - Workspace picker control plane for the shell (#376). - Add `llm-router` and `workflow` dependencies to iii.worker.yaml and workflow.yaml; remove outdated observability and workflow plan docs. Claude-Session: https://claude.ai/code/session_019pMG6VY8cKLW2EzF1FEkV9 * fix(workflow): address PR review findings - start: fold each node's input.from/fanout.over reads into depends_on before validate+persist, so a node never schedules before a node it reads (was: the read resolved to null while the run still reported success); reserve '/' in node ids too (it's the run_id/node_uid storage-key separator). - state(delete_run): propagate child deletes so the run record survives as the retry anchor on a transient failure, and clear the session reverse-index so GC stops leaking index rows. - locks: hold Weak (not Arc) per-run mutexes so a finished run's lock frees instead of pinning forever. - configuration: release the harness-hook bind claim on a partial bind so a later registry-change event retries; commit a new sweep_expression only after the cron rebind actually succeeds. - tick(build_opening): escape angle brackets in the untrusted fenced input so upstream output can't forge a </workflow_input> break-out. - events(reply): drop the dead top-level `run` field — harness::send has no such field (verified against harness send.rs) and always drives a turn; the captured caller policy already rides in options.functions. - node-result: serialize an explicit `{ "result": null }` instead of dropping the key, matching the documented contract. - main: warn and fall back to WorkerConfig::default() on a config-fetch error instead of aborting boot (matches the other worker binaries). - inject_guidance: correct the default-node-policy wording (reach minus the workflow control plane), and README: sdk 0.19.2 -> 0.20.0 (pinned iii-sdk).
This was referenced Jul 2, 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
3 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
shell::workspace::{roots,list,validate}APIs for browsing and validating host directoriesDirectoryPickerfromcoder::*discovery to the new workspace control planebase_dirinjection and fallback agent exposurebase_dirselections outside configured roots while preserving relative escape rejectionbase_dirfields from published schemas and update coder schema goldensReplaces #375 with an independent implementation.
Validation
cargo test --manifest-path harness/Cargo.toml workspace_inject --libcargo test --manifest-path shell/Cargo.toml workspace::tests --libcargo test --manifest-path shell/Cargo.toml session_scoped_base_dir_outside_roots --libcargo test --manifest-path shell/Cargo.toml base_dir_outside_host_root --libcargo test --manifest-path shell/Cargo.toml relative_base_dir_still_cannot_escape_host_root --libcargo test --manifest-path shell/Cargo.toml --test code_golden_schemascargo test --manifest-path shell/Cargo.toml --bin shell config_defaults_to_local_config_yamlpnpm --dir console/web test -- DirectoryPicker real-metadatapnpm --dir console/web typecheckSummary by CodeRabbit
New Features
Bug Fixes