opencode: allow nested workdir picker paths#791
Open
Datawav wants to merge 1 commit into
Open
Conversation
Extend the workdir picker to accept multi-segment paths like `/projects/mdk` in addition to `/mdk`. Each `/`-separated segment still uses the existing ASCII-alphanumeric + `._-` charset and rejects empty, `.`, and `..` segments. Replace the parent-equals check in `resolve_repo` and `validate_session_cwd` with a canonical `starts_with` ancestor check so nested targets resolve inside $HOME while symlinks that escape are still rejected after canonicalization. Add tests for nested acceptance, expanded empty/dot segment rejection, direct $HOME rejection, and symlink escape rejection for both entry points.
|
Ready to review this PR? Stage has broken it down into 3 individual chapters for you:
Chapters generated by Stage for commit b1dfcbd on Jul 8, 2026 10:51am UTC. |
WalkthroughThis PR extends the OpenCode marmot repo picker to accept nested ChangesNested Repo Picker Path Support
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
integrations/opencode/marmot/src/repo_picker.rs (1)
44-48: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winError message is misleading when the target is
$HOMEitself.When
canonical == home_canonical(e.g., name"."), the error says "resolves outside $HOME" but the path actually resolves to$HOME. Consider a distinct message for the direct-$HOMEcase.💡 Proposed fix
if canonical == home_canonical || !canonical.starts_with(&home_canonical) { - return Err(repo_error(format!( - "Repo ~/{name} resolves outside $HOME; refusing." - ))); + if canonical == home_canonical { + return Err(repo_error(format!( + "Repo ~/{name} resolves to $HOME; refusing." + ))); + } + return Err(repo_error(format!( + "Repo ~/{name} resolves outside $HOME; refusing." + ))); }🤖 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 `@integrations/opencode/marmot/src/repo_picker.rs` around lines 44 - 48, The validation in repo_picker::pick_repo uses one error message for both cases, but canonical == home_canonical is not “outside $HOME” and needs its own branch. Update the conditional around the canonical/home_canonical check so direct resolution to $HOME (for example from "." or equivalent) reports a distinct, accurate message, while the existing message remains for paths that resolve outside $HOME.
🤖 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.
Outside diff comments:
In `@integrations/opencode/marmot/src/repo_picker.rs`:
- Around line 44-48: The validation in repo_picker::pick_repo uses one error
message for both cases, but canonical == home_canonical is not “outside $HOME”
and needs its own branch. Update the conditional around the
canonical/home_canonical check so direct resolution to $HOME (for example from
"." or equivalent) reports a distinct, accurate message, while the existing
message remains for paths that resolve outside $HOME.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 166833f3-61ea-4738-aab2-b8f3e7027f4c
📒 Files selected for processing (2)
integrations/opencode/marmot/README.mdintegrations/opencode/marmot/src/repo_picker.rs
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
1 participant
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.
What
Extend the
wn-opencodeworkdir picker to accept nested paths under$HOME, so users can send/projects/mdk fix the failing testin addition to the existing/mdkshape.Why
The picker previously required the selected directory to be a direct child of
$HOME. Repos commonly live one or more levels deeper (e.g.~/projects/mdk,~/work/whitenoise), and the only workaround was a top-level symlink. Nested selection is the natural user-facing extension.How
parse_repo_pickersplits the picker on/and applies the existing charset rule per segment. Empty segments,., and..are rejected at parse time.resolve_repoandvalidate_session_cwdnow use a canonicalstarts_with($HOME)ancestor check instead of aparent()equality check. Direct$HOMEselection is still rejected inresolve_repo.tokio::fs::canonicalizebefore the ancestor check, so a symlink whose target escapes$HOMEis rejected.Tests
cargo test -p wn-opencode— 36 unit tests pass (was 31). New coverage:parse_repo_picker_matches_nested_pathparse_repo_picker_rejects_bare_slash_or_empty_segments(expanded)parse_repo_picker_rejects_dot_segments(expanded with nested../.)validate_session_cwd_allows_home_or_nested_child(expanded)resolve_repo_accepts_nested_pathresolve_repo_rejects_direct_home_targetresolve_repo_rejects_symlink_escaping_home(unix)validate_session_cwd_rejects_symlink_escaping_home(unix)cargo fmt -p wn-opencode -- --checkclean.cargo clippy -p wn-opencode --all-targets -- -D warningsclean.Docs
README workdir picker section updated with the nested example and the symlink-canonicalization note.
Scope
No changes to
bridge.rs, control protocol, chunking, session store, or the installer. Behavior for single-segment pickers is unchanged.Summary by CodeRabbit
New Features
/<path>instead of only direct child directories.Bug Fixes
.and..path segments, and invalid characters.~are accepted, while paths that resolve outside~are still blocked, including through symlinks.