feat: add clone path and related test#752
Conversation
|
Click here to review and test in web IDE: |
&message=Dev%20cluster%20(for%20maintainers)&logo=eclipseche&color=525C86&labelColor=FDB940)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds optional Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant CodeWorkspace
participant path
participant FileSystem
CodeWorkspace->>path: resolve(PROJECTS_ROOT, clonePath or name)
CodeWorkspace->>CodeWorkspace: validate resolved path under PROJECTS_ROOT
CodeWorkspace->>FileSystem: check resolved path exists
CodeWorkspace->>CodeWorkspace: add workspace folder with resolved path
Suggested reviewers: 🚥 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.
Actionable comments posted: 2
🤖 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 `@launcher/src/code-workspace.ts`:
- Around line 156-161: `clonePath` in `code-workspace.ts` is being used directly
in `workspace.folders` path construction, which allows path traversal outside
`env.PROJECTS_ROOT`. Update the workspace-building logic around `pathProject` so
`clonePath` is validated/sanitized to ensure it stays relative to the projects
root before calling `fs.pathExists` or pushing a folder. Use the
`workspace.folders` append flow and the `project.clonePath || project.name`
assignment as the main place to enforce the check, and reject or ignore any
value that resolves outside `env.PROJECTS_ROOT`.
In `@launcher/tests/code-workspace.spec.ts`:
- Around line 525-557: Add a test in CodeWorkspace.generate() for the existing
single-project workspace case where the .code-workspace file already exists
under PROJECTS_ROOT using clonePath, not project.name. Update the
pathExists/readFile setup to return true for ${clonePath}/.code-workspace and
verify generate() looks up that exact location. This should cover the branch
that currently uses the wrong lookup key and ensure no writeFile occurs when the
workspace already exists.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 58ef240a-5a03-4d01-8b05-e7b1da04b9a5
📒 Files selected for processing (4)
launcher/src/code-workspace.tslauncher/src/flattened-devfile.tslauncher/tests/_data/flattened.devworkspace.with-clone-path.yamllauncher/tests/code-workspace.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
launcher/src/code-workspace.ts (1)
162-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
constinstead ofletforpathProject.
pathProjectis never reassigned.♻️ Proposed fix
- let pathProject = project.clonePath || project.name; + const pathProject = project.clonePath || project.name;🤖 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 `@launcher/src/code-workspace.ts` at line 162, Use const instead of let for pathProject in the code-workspace logic because the value is never reassigned. Update the declaration in the code that builds the workspace path (around the pathProject assignment) so it remains immutable and matches the existing naming in this section.
🤖 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 `@launcher/src/code-workspace.ts`:
- Around line 163-167: The workspace-folder path validation in code-workspace.ts
is using a string-prefix check that can be bypassed by paths like sibling
directories with the same prefix. Update the fullPath validation in the
project-iteration logic to use a real path-boundary check based on path.relative
against env.PROJECTS_ROOT, and keep the existing skip/continue behavior when the
resolved path escapes the root.
---
Nitpick comments:
In `@launcher/src/code-workspace.ts`:
- Line 162: Use const instead of let for pathProject in the code-workspace logic
because the value is never reassigned. Update the declaration in the code that
builds the workspace path (around the pathProject assignment) so it remains
immutable and matches the existing naming in this section.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8bf1dbc1-438d-44fc-8113-055887b34452
📒 Files selected for processing (4)
launcher/src/code-workspace.tslauncher/src/flattened-devfile.tslauncher/tests/_data/flattened.devworkspace.with-clone-path.yamllauncher/tests/code-workspace.spec.ts
✅ Files skipped from review due to trivial changes (1)
- launcher/tests/_data/flattened.devworkspace.with-clone-path.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- launcher/src/flattened-devfile.ts
- launcher/tests/code-workspace.spec.ts
d0cac29 to
11c0d7c
Compare
Signed-off-by: Max Batleforc <maxleriche.60@gmail.com> Assisted-by: CodeRabbit review
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-752-amd64 |
|
Test looks ok, just fast forward to be in sync with main. OK for me @RomanNikitenko if you have time to review |
1 participant
What does this PR do?
In code-workspace, either use the clonePath if it exist or the project name to find the folder.
What issues does this PR fix?
eclipse-che/che#23909
How to test this PR?
Does this PR contain changes that override default upstream Code-OSS behavior?
git rebasewere added to the .rebase folderSummary by CodeRabbit
New Features
Bug Fixes
clonePath(fallback to project name), verifies they remain under the configured root, and safely skips invalid/out-of-root paths.Tests
clonePathand expanded coverage for generation and synchronization scenarios.