Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes the "text file busy" error that occurs when running make install while the picoclaw binary is currently running. The solution implements an atomic file replacement pattern using a temporary file and mv, which is a standard approach for replacing executables that may be in use.
Changes:
- Adds a temporary file suffix variable (
INSTALL_TMP_SUFFIX=.new) - Modifies the install target to use atomic file replacement (copy to temp → chmod → atomic move)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zenixls2
force-pushed
the
fix/ota-textfile-busy
branch
from
18e7fdb to
255ebbb
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err := os.MkdirAll(workspace, 0o755); err != nil { | ||
| t.Fatalf("failed to create workspace: %v", err) | ||
| } | ||
| if err := os.MkdirAll(outsideDir, 0755); err != nil { | ||
| if err := os.MkdirAll(outsideDir, 0o755); err != nil { | ||
| t.Fatalf("failed to create outside dir: %v", err) | ||
| } | ||
|
|
||
| tool := NewExecTool(workspace, true) | ||
| result := tool.Execute(context.Background(), map[string]interface{}{ | ||
| result := tool.Execute(context.Background(), map[string]any{ | ||
| "command": "pwd", | ||
| "working_dir": outsideDir, | ||
| }) | ||
|
|
||
| if !result.IsError { | ||
| t.Fatalf("expected working_dir outside workspace to be blocked, got output: %s", result.ForLLM) | ||
| } | ||
| if !strings.Contains(result.ForLLM, "blocked") { | ||
| t.Errorf("expected 'blocked' in error, got: %s", result.ForLLM) | ||
| } | ||
| } | ||
|
|
||
| // TestShellTool_WorkingDir_SymlinkEscape verifies that a symlink inside the workspace | ||
| // pointing outside cannot be used as working_dir to escape the sandbox. | ||
| func TestShellTool_WorkingDir_SymlinkEscape(t *testing.T) { | ||
| root := t.TempDir() | ||
| workspace := filepath.Join(root, "workspace") | ||
| secretDir := filepath.Join(root, "secret") | ||
| if err := os.MkdirAll(workspace, 0755); err != nil { | ||
| if err := os.MkdirAll(workspace, 0o755); err != nil { | ||
| t.Fatalf("failed to create workspace: %v", err) | ||
| } | ||
| if err := os.MkdirAll(secretDir, 0755); err != nil { | ||
| if err := os.MkdirAll(secretDir, 0o755); err != nil { | ||
| t.Fatalf("failed to create secret dir: %v", err) | ||
| } | ||
| os.WriteFile(filepath.Join(secretDir, "secret.txt"), []byte("top secret"), 0644) | ||
| os.WriteFile(filepath.Join(secretDir, "secret.txt"), []byte("top secret"), 0o644) |
There was a problem hiding this comment.
The changes to shell_test.go (octal literal format and map type alias) appear to be unrelated to the PR's stated purpose of fixing "text file busy" errors during installation. These are code style improvements that should be mentioned in the PR description or submitted as a separate PR. The PR description claims this is a bug fix for the install target, but these test file changes are purely stylistic improvements that don't relate to the installation issue.
… to overwrite the file with non-atomic operation
zenixls2
force-pushed
the
fix/ota-textfile-busy
branch
from
539ca48 to
2893448
Compare
1 participant
📝 Description
make install from bot will get textfile busy message, since it's still running
🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
📚 Technical Context (Skip for Docs)
🧪 Test Environment
📸 Evidence (Optional)
Click to view Logs/Screenshots
☑️ Checklist