picod: properly initialize workspace directory when --workspace is set #170
Conversation
volcano-sh-bot
requested review from
LiZhenCheng9527,
YaoZengzeng and
hzxuzhonghu
|
Welcome @aaradhychinche-alt! It looks like this is your first PR to volcano-sh/agentcube 🎉 |
Summary of ChangesHello @aaradhychinche-alt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of picod failing when a custom workspace directory doesn't exist. The changes to initialize and validate the workspace at startup are robust. My review includes a few suggestions for improvement:
- In
pkg/picod/files.go, I've pointed out a redundant check that can be removed to simplify the code. More importantly, the implementation is missing the logic to change the process's current working directory, which is a behavior described in both the PR description and the updated documentation. I've suggested adding this to align the code with the documentation. - In
pkg/picod/execute.go, I've recommended creating the specified working directory if it doesn't exist, instead of returning an error. This would provide a more consistent and user-friendly API, aligning its behavior with the file upload endpoints.
Overall, these are solid changes that improve the reliability of workspace handling.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #170 +/- ##
==========================================
+ Coverage 35.60% 35.95% +0.34%
==========================================
Files 29 29
Lines 2533 2564 +31
==========================================
+ Hits 902 922 +20
- Misses 1505 1515 +10
- Partials 126 127 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
hzxuzhonghu
left a comment
hzxuzhonghu
left a comment
There was a problem hiding this comment.
you can test this feature with e2e. Only need to update ./test/e2e/e2e_code_interpreter.yaml
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #154 where the picod service fails to execute code when started with a custom --workspace directory that doesn't exist in the container. The fix ensures that workspace directories are properly initialized, created if missing, and set as the process working directory at startup.
Changes:
- Modified
setWorkspaceto create workspace directories automatically, validate they exist, and change the process working directory - Added workspace directory creation in
ExecuteHandlerwhen a custom working directory is specified in execution requests - Updated tests to properly save and restore working directories to avoid side effects
- Enhanced documentation and examples to clarify workspace behavior and add volume mount configuration
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/picod/files.go | Enhanced setWorkspace function to create directories, validate paths, save original working directory, and change process working directory to workspace |
| pkg/picod/server.go | Added originalWorkingDir field and RestoreWorkingDirectory method for test cleanup |
| pkg/picod/execute.go | Added directory creation guard in ExecuteHandler to ensure working directories exist before command execution |
| pkg/picod/picod_test.go | Updated tests to capture and restore working directory to prevent side effects; added test case for non-existent working directory creation |
| example/code-interpreter/code-interpreter.yaml | Changed workspace from /root to /workspace with proper emptyDir volume mount configuration |
| docs/getting-started.md | Added note explaining automatic workspace directory creation and working directory behavior |
pkg/picod/files.go
Outdated
| // Verify path exists and is a directory | ||
| stat, err := os.Stat(absDir) | ||
| if err != nil { | ||
| klog.Fatalf("failed to stat workspace directory %q: %v", absDir, err) | ||
| } | ||
| if !stat.IsDir() { | ||
| klog.Fatalf("workspace path %q is not a directory", absDir) | ||
| } | ||
|
|
There was a problem hiding this comment.
After successfully calling os.MkdirAll on line 410, which creates the directory with mode 0755, the subsequent os.Stat check on lines 415-421 is redundant. The os.MkdirAll function will either succeed (meaning the directory exists and is valid) or return an error that's already being handled with klog.Fatalf on line 411. The only case where stat.IsDir() could return false is if a file (not a directory) was created at that path between lines 410 and 415, which is highly unlikely in typical startup scenarios. Consider removing lines 414-421 to simplify the code, or add a comment explaining why this extra validation is necessary if there's a specific security or operational concern.
| // Verify path exists and is a directory | |
| stat, err := os.Stat(absDir) | |
| if err != nil { | |
| klog.Fatalf("failed to stat workspace directory %q: %v", absDir, err) | |
| } | |
| if !stat.IsDir() { | |
| klog.Fatalf("workspace path %q is not a directory", absDir) | |
| } |
pkg/picod/server.go
Outdated
| // RestoreWorkingDirectory restores the process working directory to its original state | ||
| func (s *Server) RestoreWorkingDirectory() { | ||
| if s.originalWorkingDir != "" { | ||
| if err := os.Chdir(s.originalWorkingDir); err != nil { | ||
| klog.Warningf("failed to restore working directory to %q: %v", s.originalWorkingDir, err) | ||
| } else { | ||
| klog.Infof("restored working directory to: %q", s.originalWorkingDir) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The RestoreWorkingDirectory method is added but never called in the production code path. While it's used in tests (e.g., line 397 in picod_test.go), there's no corresponding defer or cleanup in the main.go entry point for picod. This means if the picod process terminates gracefully, the working directory won't be restored. This is likely not a practical issue since the process is terminating anyway, but if the intent is to support cleanup scenarios, consider either: (1) calling it in main.go with a signal handler, or (2) documenting that this method is primarily for testing purposes.
pkg/picod/files.go
Outdated
| klog.Fatalf("failed to resolve absolute path for workspace %q: %v", dir, err) | ||
| } | ||
|
|
||
| // Create directory if it doesn't exist | ||
| if err := os.MkdirAll(absDir, 0755); err != nil { | ||
| klog.Fatalf("failed to create workspace directory %q: %v", absDir, err) | ||
| } | ||
|
|
||
| // Verify path exists and is a directory | ||
| stat, err := os.Stat(absDir) | ||
| if err != nil { | ||
| klog.Fatalf("failed to stat workspace directory %q: %v", absDir, err) | ||
| } | ||
| if !stat.IsDir() { | ||
| klog.Fatalf("workspace path %q is not a directory", absDir) | ||
| } | ||
|
|
||
| // Set workspace directory | ||
| s.workspaceDir = absDir | ||
|
|
||
| // Change process working directory to workspace | ||
| if err := os.Chdir(absDir); err != nil { | ||
| klog.Fatalf("failed to change working directory to %q: %v", absDir, err) |
There was a problem hiding this comment.
Error messages in this function should start with uppercase letters to match the convention established elsewhere in this codebase. The existing error messages in server.go use uppercase (e.g., "Failed to get current working directory" on line 62, "Failed to load public key from environment" on line 80). These new error messages use lowercase ("failed to resolve absolute path", "failed to create workspace directory", etc.). Please capitalize the first letter of each error message to maintain consistency with the rest of the picod package.
pkg/picod/files.go
Outdated
| if s.originalWorkingDir == "" { | ||
| cwd, err := os.Getwd() | ||
| if err != nil { | ||
| klog.Warningf("failed to get current working directory: %v", err) |
There was a problem hiding this comment.
The error messages in this function should start with uppercase letters to match the convention in the same file. For example, line 397 uses lowercase "failed to get current working directory" while the existing code on lines 355, 379, 383 uses uppercase messages like "Failed to get info for entry", "Invalid file mode". Please capitalize the first letter to maintain consistency: "Failed to get current working directory".
pkg/picod/server.go
Outdated
| klog.Warningf("failed to restore working directory to %q: %v", s.originalWorkingDir, err) | ||
| } else { | ||
| klog.Infof("restored working directory to: %q", s.originalWorkingDir) |
There was a problem hiding this comment.
The error messages in this function should start with uppercase letters to match the convention established in server.go. Error messages on line 118 ("failed to restore working directory") and line 120 ("restored working directory") use lowercase, but the existing code in server.go uses uppercase for error messages (e.g., "Failed to get current working directory" on line 62). Please capitalize: "Failed to restore working directory" and "Restored working directory".
Signed-off-by: Aaradhy Chinche <aaradhychinche@gmail.com>
Signed-off-by: Aaradhy Chinche <aaradhychinche@gmail.com>
Signed-off-by: Aaradhy Chinche <aaradhychinche@gmail.com>
Signed-off-by: Aaradhy Chinche <aaradhychinche@gmail.com>
aaradhychinche-alt
force-pushed
the
fix/picod-workspace-init
branch
from
92454db to
c12162e
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: Aaradhy Chinche <aaradhychinche@gmail.com>
Signed-off-by: Aaradhy Chinche <aaradhychinche@gmail.com>
Signed-off-by: Aaradhy Chinche <aaradhychinche@gmail.com>
|
@hzxuzhonghu Updated docs to no longer claim the process working directory is changed Removed redundant os.Stat checks after os.MkdirAll Please take another look |
pkg/picod/files.go
Outdated
| // Set workspace directory | ||
| s.workspaceDir = absDir | ||
|
|
||
| klog.Infof("workspace directory initialized: %q", s.workspaceDir) |
There was a problem hiding this comment.
The PR description states "Sets the process working directory to the workspace", but there is no call to os.Chdir(absDir) to actually change the process's working directory. Without this, commands executed without an explicit WorkingDir will run in the picod process's initial working directory rather than the configured workspace. Consider adding if err := os.Chdir(absDir); err != nil { klog.Fatalf("failed to change to workspace directory %q: %v", absDir, err) } after line 402.
| self.session = requests.Session() | ||
|
|
||
| def _sign_jwt(self, claims: dict = None) -> str: | ||
| def _sign_jwt(self, claims: Optional[Dict[str, Any]] = None) -> str: |
There was a problem hiding this comment.
This change adds type hints to the _sign_jwt method but doesn't relate to the workspace initialization fix described in the PR. While the type hint improvement is fine, it should ideally be in a separate PR focused on type hint improvements rather than mixed with an unrelated bug fix.
pkg/picod/execute.go
Outdated
| // Ensure working directory exists | ||
| if err := os.MkdirAll(safeWorkingDir, 0755); err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{ | ||
| "error": fmt.Sprintf("failed to create working directory: %v", err), |
There was a problem hiding this comment.
The error message "failed to create working directory" starts with a lowercase letter, which is inconsistent with other error messages in this file that start with uppercase letters (e.g., "Invalid working directory" on line 100, "Invalid timeout format" on line 80). For consistency with the existing codebase style, this should be capitalized as "Failed to create working directory".
| // setWorkspace sets the global workspace directory | ||
| func (s *Server) setWorkspace(dir string) { | ||
| klog.Infof("setWorkspace called with dir: %q", dir) | ||
|
|
||
| // Resolve to absolute path | ||
| absDir, err := filepath.Abs(dir) | ||
| if err != nil { | ||
| klog.Warningf("Failed to resolve absolute path for workspace '%s': %v", dir, err) | ||
| s.workspaceDir = dir // Fallback to provided path | ||
| } else { | ||
| s.workspaceDir = absDir | ||
| klog.Infof("Resolved workspace to absolute path: %q", s.workspaceDir) | ||
| klog.Fatalf("failed to resolve absolute path for workspace %q: %v", dir, err) | ||
| } | ||
|
|
||
| // Create directory if it doesn't exist | ||
| if err := os.MkdirAll(absDir, 0755); err != nil { | ||
| klog.Fatalf("failed to create workspace directory %q: %v", absDir, err) | ||
| } | ||
|
|
||
| // Set workspace directory | ||
| s.workspaceDir = absDir | ||
|
|
||
| klog.Infof("workspace directory initialized: %q", s.workspaceDir) | ||
| } |
There was a problem hiding this comment.
The PR description states "Sets the process working directory to the workspace", but there is no os.Chdir call in the implementation to actually change the process's working directory. The current implementation only creates the workspace directory and stores its path in s.workspaceDir. This means commands executed without an explicit WorkingDir in the request will run in the process's original working directory, not the configured workspace. Consider either adding os.Chdir(absDir) after line 402 to match the PR description, or updating the PR description to accurately reflect the implementation.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
pkg/picod/picod_test.go:466
- The TestPicoD_SetWorkspace test only tests cases where the directory already exists (created with os.Mkdir on line 425). Consider adding a test case that verifies the new functionality where setWorkspace creates a non-existent directory. This would better validate the fix for issue #154.
func TestPicoD_SetWorkspace(t *testing.T) {
// Capture current working directory and restore it in cleanup
originalWd, err := os.Getwd()
require.NoError(t, err)
t.Cleanup(func() {
if err := os.Chdir(originalWd); err != nil {
t.Logf("failed to restore working directory: %v", err)
}
})
// Setup temp dir
tmpDir, err := os.MkdirTemp("", "picod_setworkspace_test")
require.NoError(t, err)
defer os.RemoveAll(tmpDir)
// Create a real directory
realDir := filepath.Join(tmpDir, "real")
err = os.Mkdir(realDir, 0755)
require.NoError(t, err)
// Create a symlink
linkDir := filepath.Join(tmpDir, "link")
err = os.Symlink(realDir, linkDir)
require.NoError(t, err)
server := &Server{}
// Helper to resolve path for comparison (handles /var vs /private/var on macOS)
resolve := func(p string) string {
path, err := filepath.EvalSymlinks(p)
if err != nil {
return p
}
path, err = filepath.Abs(path)
if err != nil {
return path
}
return path
}
// Case 1: Absolute Path
absPath, err := filepath.Abs(realDir)
require.NoError(t, err)
server.setWorkspace(realDir)
assert.Equal(t, resolve(absPath), resolve(server.workspaceDir))
// Case 2: Relative Path
err = os.Chdir(tmpDir)
require.NoError(t, err)
server.setWorkspace("real")
assert.Equal(t, resolve(absPath), resolve(server.workspaceDir))
// Case 3: Symlink
absLinkPath, err := filepath.Abs(linkDir)
require.NoError(t, err)
server.setWorkspace(linkDir)
assert.Equal(t, resolve(absLinkPath), resolve(server.workspaceDir))
}
|
/lgtm |
|
@acsoto: changing LGTM is restricted to collaborators DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Signed-off-by: Aaradhy Chinche <aaradhychinche@gmail.com>
|
please check the e2e failure, i think it is related |
working on it |
| assert.NotEmpty(t, nonExistResp.Stdout) | ||
|
|
||
| // Verify directory was created | ||
| _, err = os.Stat("subdir/nested") |
There was a problem hiding this comment.
where do we cleanup this dir?
| self.session = requests.Session() | ||
|
|
||
| def _sign_jwt(self, claims: dict = None) -> str: | ||
| def _sign_jwt(self, claims: Optional[Dict[str, Any]] = None) -> str: |
There was a problem hiding this comment.
Seems not related, canyou revert
| } | ||
|
|
||
| // Ensure working directory exists | ||
| if err := os.MkdirAll(safeWorkingDir, 0755); err != nil { |
There was a problem hiding this comment.
Would return error if user doesn't specify workspace? I mean it would be /root actually which already exist.
pkg/picod/files.go
Outdated
| } | ||
|
|
||
| // Create directory if it doesn't exist | ||
| if err := os.MkdirAll(absDir, 0755); err != nil { |
There was a problem hiding this comment.
The workspace creation should be make in picod initialization (https://github.com/volcano-sh/agentcube/blob/main/pkg/picod/server.go#L54), not everytime we call these execute/file related API.
…ments Signed-off-by: Aaradhy Chinche <aaradhychinche@gmail.com>
…ments Signed-off-by: Aaradhy Chinche <aaradhychinche@gmail.com>
| // hasAbsolutePathArg checks if any command argument (starting from index 1) is an absolute path. | ||
| func hasAbsolutePathArg(args []string) bool { | ||
| for i := 1; i < len(args); i++ { | ||
| if filepath.IsAbs(args[i]) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
The hasAbsolutePathArg function and its usage (lines 128-140) appears to introduce problematic behavior. When a command has an absolute path in its arguments (e.g., ["cat", "/etc/hosts"]), the working directory is not set. This is incorrect because:
- Absolute paths in arguments don't conflict with setting cmd.Dir - they will resolve correctly regardless
- Not setting cmd.Dir means relative paths in the command won't resolve relative to the workspace
- There's no test coverage for this logic to validate its necessity
If there's a specific issue that this logic is trying to address, it should be documented and tested. Otherwise, cmd.Dir should always be set to safeWorkingDir to ensure consistent behavior.
| // hasAbsolutePathArg checks if any command argument (starting from index 1) is an absolute path. | |
| func hasAbsolutePathArg(args []string) bool { | |
| for i := 1; i < len(args); i++ { | |
| if filepath.IsAbs(args[i]) { | |
| return true | |
| } | |
| } | |
| return false | |
| } | |
| // hasAbsolutePathArg is retained for backward compatibility but no longer influences | |
| // working directory selection. Absolute paths in arguments do not conflict with | |
| // setting a working directory, so this function always returns false. | |
| func hasAbsolutePathArg(args []string) bool { | |
| return false | |
| } | |
| } |
| // Check if any command argument is an absolute path. | ||
| // If so, don't set cmd.Dir to avoid issues with absolute paths. | ||
| // Otherwise, set cmd.Dir to the workspace for relative path resolution. | ||
| hasAbsolutePath := hasAbsolutePathArg(req.Command) | ||
|
|
||
| // Execute command with context | ||
| // Use the first element as the command and the rest as arguments | ||
| cmd := exec.CommandContext(ctx, req.Command[0], req.Command[1:]...) //nolint:gosec // This is an agent designed to execute arbitrary commands | ||
|
|
||
| // Set working directory | ||
| if req.WorkingDir != "" { | ||
| safeWorkingDir, err := s.sanitizePath(req.WorkingDir) | ||
| if err != nil { | ||
| c.JSON(http.StatusBadRequest, gin.H{ | ||
| "error": fmt.Sprintf("Invalid working directory: %v", err), | ||
| "code": http.StatusBadRequest, | ||
| }) | ||
| return | ||
| } | ||
| // Only set cmd.Dir if no absolute paths are in the arguments | ||
| if !hasAbsolutePath { | ||
| cmd.Dir = safeWorkingDir | ||
| } |
There was a problem hiding this comment.
The logic that conditionally sets cmd.Dir based on whether arguments contain absolute paths (lines 128-140) is problematic. Setting cmd.Dir should not depend on the presence of absolute paths in command arguments, as:
- Absolute paths in arguments work correctly whether or not cmd.Dir is set
- Not setting cmd.Dir when absolute path arguments are present breaks relative path handling for the command itself
- The comment "avoid issues with absolute paths" doesn't describe what specific issues this prevents
The code should always set cmd.Dir = safeWorkingDir (lines 137-140 should be simplified to just set cmd.Dir unconditionally).
| // hasAbsolutePathArg checks if any command argument (starting from index 1) is an absolute path. | ||
| func hasAbsolutePathArg(args []string) bool { | ||
| for i := 1; i < len(args); i++ { | ||
| if filepath.IsAbs(args[i]) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
The hasAbsolutePathArg function lacks test coverage. Given that this function impacts the execution behavior (whether cmd.Dir is set), it should have explicit test cases covering:
- Commands with absolute path arguments (e.g., ["cat", "/etc/hosts"])
- Commands with only relative path arguments
- Edge cases like empty strings or paths starting with "./"
This is especially important since the logic appears questionable (see related comments on lines 56-64 and 128-140).
This commit fixes a critical bug where workspace paths containing symlinks (like /var -> /private/var on macOS) were being incorrectly duplicated when processing file operations and commands. The problem occurred in three places: 1. setWorkspace() was resolving paths with filepath.Abs() but not filepath.EvalSymlinks(), leading to inconsistent path representations 2. sanitizePath() was incorrectly treating absolute paths within the workspace as if they were relative paths, resulting in path duplication (e.g., /root/root/file instead of /root/file) The fixes: 1. Updated setWorkspace() to resolve symlinks using filepath.EvalSymlinks() after filepath.Abs(), ensuring consistent canonical path storage 2. Updated sanitizePath() to properly handle absolute paths: - If input is absolute and within workspace: return as-is (after validation) - If input is absolute and outside workspace: reject with error - If input is relative: construct path within workspace (existing behavior) 3. Added comprehensive test case simulating Python SDK workflow: - Create Python script file with relative path - Execute script with relative path argument - Verify proper file creation and execution This ensures proper path handling across different systems with varying symlink configurations, particularly important for CI environments. Fixes the E2E test failures with error: 'python3: can't open file /root/root/script_XXX.py: [Errno 2] No such file' Signed-off-by: Aaradhy Chinche <aaradhychinche@gmail.com>
aaradhychinche-alt
force-pushed
the
fix/picod-workspace-init
branch
from
47dec48 to
c849b80
Compare
6 participants
Fixes issue #154
where picod failed to execute code when started with a
custom
--workspacedirectory (for example/workspace) that did notalready exist inside the container.
The workspace path was previously recorded but not created or validated,
which caused file operations and command execution to fail at runtime.
This change:
and volume mounting