renato97/picoclaw
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge upstream/main into ralph/tool-result-refactor

Browse Source 
Resolved conflicts:
- pkg/heartbeat/service.go: merged both 'started' field and 'onHeartbeatWithTools'
- pkg/tools/edit.go: use validatePath() with ToolResult return
- pkg/tools/filesystem.go: fixed return values to use ToolResult
- cmd/picoclaw/main.go: kept active setupCronTool, fixed toolsPkg import
- pkg/tools/cron.go: fixed Execute return value handling

Fixed tests for new function signatures (NewEditFileTool, NewAppendFileTool, NewExecTool)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
yinwm
2026-02-13 01:00:26 +08:00
parent b36c87bd60 d165fde011
commit ab20314882
25 changed files with 2049 additions and 202 deletions
Download Patch File Download Diff File Expand all files Collapse all files

51
pkg/tools/cron.go
View File

@@ -1,4 +1,4 @@
package tools
package tools
import (
"context"
@@ -21,17 +21,19 @@ type CronTool struct {
cronService *cron.CronService
executor JobExecutor
msgBus *bus.MessageBus
execTool *ExecTool
channel string
chatID string
mu sync.RWMutex
}
// NewCronTool creates a new CronTool
func NewCronTool(cronService *cron.CronService, executor JobExecutor, msgBus *bus.MessageBus) *CronTool {
func NewCronTool(cronService *cron.CronService, executor JobExecutor, msgBus *bus.MessageBus, workspace string) *CronTool {
return &CronTool{
cronService: cronService,
executor: executor,
msgBus: msgBus,
execTool: NewExecTool(workspace, false),
}
}
@@ -42,7 +44,7 @@ func (t *CronTool) Name() string {
// Description returns the tool description
func (t *CronTool) Description() string {
return "Schedule reminders and tasks. IMPORTANT: When user asks to be reminded or scheduled, you MUST call this tool. Use 'at_seconds' for one-time reminders (e.g., 'remind me in 10 minutes' → at_seconds=600). Use 'every_seconds' ONLY for recurring tasks (e.g., 'every 2 hours' → every_seconds=7200). Use 'cron_expr' for complex recurring schedules (e.g., '0 9 * * *' for daily at 9am)."
return "Schedule reminders, tasks, or system commands. IMPORTANT: When user asks to be reminded or scheduled, you MUST call this tool. Use 'at_seconds' for one-time reminders (e.g., 'remind me in 10 minutes' → at_seconds=600). Use 'every_seconds' ONLY for recurring tasks (e.g., 'every 2 hours' → every_seconds=7200). Use 'cron_expr' for complex recurring schedules. Use 'command' to execute shell commands directly."
}
// Parameters returns the tool parameters schema
@@ -57,7 +59,11 @@ func (t *CronTool) Parameters() map[string]interface{} {
},
"message": map[string]interface{}{
"type": "string",
"description": "The reminder/task message to display when triggered (required for add)",
"description": "The reminder/task message to display when triggered. If 'command' is used, this describes what the command does.",
},
"command": map[string]interface{}{
"type": "string",
"description": "Optional: Shell command to execute directly (e.g., 'df -h'). If set, the agent will run this command and report output instead of just showing the message. 'deliver' will be forced to false for commands.",
},
"at_seconds": map[string]interface{}{
"type": "integer",
@@ -165,6 +171,15 @@ func (t *CronTool) addJob(args map[string]interface{}) *ToolResult {
deliver = d
}
command, _ := args["command"].(string)
if command != "" {
// Commands must be processed by agent/exec tool, so deliver must be false (or handled specifically)
// Actually, let's keep deliver=false to let the system know it's not a simple chat message
// But for our new logic in ExecuteJob, we can handle it regardless of deliver flag if Payload.Command is set.
// However, logically, it's not "delivered" to chat directly as is.
deliver = false
}
// Truncate message for job name (max 30 chars)
messagePreview := utils.Truncate(message, 30)
@@ -179,6 +194,12 @@ func (t *CronTool) addJob(args map[string]interface{}) *ToolResult {
if err != nil {
return ErrorResult(fmt.Sprintf("Error adding job: %v", err))
}
if command != "" {
job.Payload.Command = command
// Need to save the updated payload
t.cronService.UpdateJob(job)
}
return SilentResult(fmt.Sprintf("Cron job added: %s (id: %s)", job.Name, job.ID))
}
@@ -252,6 +273,28 @@ func (t *CronTool) ExecuteJob(ctx context.Context, job *cron.CronJob) string {
chatID = "direct"
}
// Execute command if present
if job.Payload.Command != "" {
args := map[string]interface{}{
"command": job.Payload.Command,
}
result := t.execTool.Execute(ctx, args)
var output string
if result.IsError {
output = fmt.Sprintf("Error executing scheduled command: %s", result.ForLLM)
} else {
output = fmt.Sprintf("Scheduled command '%s' executed:\n%s", job.Payload.Command, result.ForLLM)
}
t.msgBus.PublishOutbound(bus.OutboundMessage{
Channel: channel,
ChatID: chatID,
Content: output,
})
return "ok"
}
// If deliver=true, send message directly without agent processing
if job.Payload.Deliver {
t.msgBus.PublishOutbound(bus.OutboundMessage{

47
pkg/tools/edit.go
View File

@@ -4,20 +4,21 @@ import (
"context"
"fmt"
"os"
"path/filepath"
"strings"
)
// EditFileTool edits a file by replacing old_text with new_text.
// The old_text must exist exactly in the file.
type EditFileTool struct {
allowedDir string // Optional directory restriction for security
allowedDir string
restrict bool
}
// NewEditFileTool creates a new EditFileTool with optional directory restriction.
func NewEditFileTool(allowedDir string) *EditFileTool {
func NewEditFileTool(allowedDir string, restrict bool) *EditFileTool {
return &EditFileTool{
allowedDir: allowedDir,
restrict: restrict,
}
}
@@ -66,27 +67,9 @@ func (t *EditFileTool) Execute(ctx context.Context, args map[string]interface{})
return ErrorResult("new_text is required")
}
// Resolve path and enforce directory restriction if configured
resolvedPath := path
if filepath.IsAbs(path) {
resolvedPath = filepath.Clean(path)
} else {
abs, err := filepath.Abs(path)
if err != nil {
return ErrorResult(fmt.Sprintf("failed to resolve path: %v", err))
}
resolvedPath = abs
}
// Check directory restriction
if t.allowedDir != "" {
allowedAbs, err := filepath.Abs(t.allowedDir)
if err != nil {
return ErrorResult(fmt.Sprintf("failed to resolve allowed directory: %v", err))
}
if !strings.HasPrefix(resolvedPath, allowedAbs) {
return ErrorResult(fmt.Sprintf("path %s is outside allowed directory %s", path, t.allowedDir))
}
resolvedPath, err := validatePath(path, t.allowedDir, t.restrict)
if err != nil {
return ErrorResult(err.Error())
}
if _, err := os.Stat(resolvedPath); os.IsNotExist(err) {
@@ -118,10 +101,13 @@ func (t *EditFileTool) Execute(ctx context.Context, args map[string]interface{})
return SilentResult(fmt.Sprintf("File edited: %s", path))
}
type AppendFileTool struct{}
type AppendFileTool struct {
workspace string
restrict bool
}
func NewAppendFileTool() *AppendFileTool {
return &AppendFileTool{}
func NewAppendFileTool(workspace string, restrict bool) *AppendFileTool {
return &AppendFileTool{workspace: workspace, restrict: restrict}
}
func (t *AppendFileTool) Name() string {
@@ -160,9 +146,12 @@ func (t *AppendFileTool) Execute(ctx context.Context, args map[string]interface{
return ErrorResult("content is required")
}
filePath := filepath.Clean(path)
resolvedPath, err := validatePath(path, t.workspace, t.restrict)
if err != nil {
return ErrorResult(err.Error())
}
f, err := os.OpenFile(filePath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
f, err := os.OpenFile(resolvedPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
if err != nil {
return ErrorResult(fmt.Sprintf("failed to open file: %v", err))
}

22
pkg/tools/edit_test.go
View File

@@ -14,7 +14,7 @@ func TestEditTool_EditFile_Success(t *testing.T) {
testFile := filepath.Join(tmpDir, "test.txt")
os.WriteFile(testFile, []byte("Hello World\nThis is a test"), 0644)
tool := NewEditFileTool(tmpDir)
tool := NewEditFileTool(tmpDir, true)
ctx := context.Background()
args := map[string]interface{}{
"path": testFile,
@@ -58,7 +58,7 @@ func TestEditTool_EditFile_NotFound(t *testing.T) {
tmpDir := t.TempDir()
testFile := filepath.Join(tmpDir, "nonexistent.txt")
tool := NewEditFileTool(tmpDir)
tool := NewEditFileTool(tmpDir, true)
ctx := context.Background()
args := map[string]interface{}{
"path": testFile,
@@ -85,7 +85,7 @@ func TestEditTool_EditFile_OldTextNotFound(t *testing.T) {
testFile := filepath.Join(tmpDir, "test.txt")
os.WriteFile(testFile, []byte("Hello World"), 0644)
tool := NewEditFileTool(tmpDir)
tool := NewEditFileTool(tmpDir, true)
ctx := context.Background()
args := map[string]interface{}{
"path": testFile,
@@ -112,7 +112,7 @@ func TestEditTool_EditFile_MultipleMatches(t *testing.T) {
testFile := filepath.Join(tmpDir, "test.txt")
os.WriteFile(testFile, []byte("test test test"), 0644)
tool := NewEditFileTool(tmpDir)
tool := NewEditFileTool(tmpDir, true)
ctx := context.Background()
args := map[string]interface{}{
"path": testFile,
@@ -140,7 +140,7 @@ func TestEditTool_EditFile_OutsideAllowedDir(t *testing.T) {
testFile := filepath.Join(otherDir, "test.txt")
os.WriteFile(testFile, []byte("content"), 0644)
tool := NewEditFileTool(tmpDir) // Restrict to tmpDir
tool := NewEditFileTool(tmpDir, true) // Restrict to tmpDir
ctx := context.Background()
args := map[string]interface{}{
"path": testFile,
@@ -163,7 +163,7 @@ func TestEditTool_EditFile_OutsideAllowedDir(t *testing.T) {
// TestEditTool_EditFile_MissingPath verifies error handling for missing path
func TestEditTool_EditFile_MissingPath(t *testing.T) {
tool := NewEditFileTool("")
tool := NewEditFileTool("", false)
ctx := context.Background()
args := map[string]interface{}{
"old_text": "old",
@@ -180,7 +180,7 @@ func TestEditTool_EditFile_MissingPath(t *testing.T) {
// TestEditTool_EditFile_MissingOldText verifies error handling for missing old_text
func TestEditTool_EditFile_MissingOldText(t *testing.T) {
tool := NewEditFileTool("")
tool := NewEditFileTool("", false)
ctx := context.Background()
args := map[string]interface{}{
"path": "/tmp/test.txt",
@@ -197,7 +197,7 @@ func TestEditTool_EditFile_MissingOldText(t *testing.T) {
// TestEditTool_EditFile_MissingNewText verifies error handling for missing new_text
func TestEditTool_EditFile_MissingNewText(t *testing.T) {
tool := NewEditFileTool("")
tool := NewEditFileTool("", false)
ctx := context.Background()
args := map[string]interface{}{
"path": "/tmp/test.txt",
@@ -218,7 +218,7 @@ func TestEditTool_AppendFile_Success(t *testing.T) {
testFile := filepath.Join(tmpDir, "test.txt")
os.WriteFile(testFile, []byte("Initial content"), 0644)
tool := NewAppendFileTool()
tool := NewAppendFileTool("", false)
ctx := context.Background()
args := map[string]interface{}{
"path": testFile,
@@ -258,7 +258,7 @@ func TestEditTool_AppendFile_Success(t *testing.T) {
// TestEditTool_AppendFile_MissingPath verifies error handling for missing path
func TestEditTool_AppendFile_MissingPath(t *testing.T) {
tool := NewAppendFileTool()
tool := NewAppendFileTool("", false)
ctx := context.Background()
args := map[string]interface{}{
"content": "test",
@@ -274,7 +274,7 @@ func TestEditTool_AppendFile_MissingPath(t *testing.T) {
// TestEditTool_AppendFile_MissingContent verifies error handling for missing content
func TestEditTool_AppendFile_MissingContent(t *testing.T) {
tool := NewAppendFileTool()
tool := NewAppendFileTool("", false)
ctx := context.Background()
args := map[string]interface{}{
"path": "/tmp/test.txt",

79
pkg/tools/filesystem.go
View File

@@ -5,9 +5,45 @@ import (
"fmt"
"os"
"path/filepath"
"strings"
)
type ReadFileTool struct{}
// validatePath ensures the given path is within the workspace if restrict is true.
func validatePath(path, workspace string, restrict bool) (string, error) {
if workspace == "" {
return path, nil
}
absWorkspace, err := filepath.Abs(workspace)
if err != nil {
return "", fmt.Errorf("failed to resolve workspace path: %w", err)
}
var absPath string
if filepath.IsAbs(path) {
absPath = filepath.Clean(path)
} else {
absPath, err = filepath.Abs(filepath.Join(absWorkspace, path))
if err != nil {
return "", fmt.Errorf("failed to resolve file path: %w", err)
}
}
if restrict && !strings.HasPrefix(absPath, absWorkspace) {
return "", fmt.Errorf("access denied: path is outside the workspace")
}
return absPath, nil
}
type ReadFileTool struct {
workspace string
restrict bool
}
func NewReadFileTool(workspace string, restrict bool) *ReadFileTool {
return &ReadFileTool{workspace: workspace, restrict: restrict}
}
func (t *ReadFileTool) Name() string {
return "read_file"
@@ -36,7 +72,12 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]interface{})
return ErrorResult("path is required")
}
content, err := os.ReadFile(path)
resolvedPath, err := validatePath(path, t.workspace, t.restrict)
if err != nil {
return ErrorResult(err.Error())
}
content, err := os.ReadFile(resolvedPath)
if err != nil {
return ErrorResult(fmt.Sprintf("failed to read file: %v", err))
}
@@ -44,7 +85,14 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]interface{})
return NewToolResult(string(content))
}
type WriteFileTool struct{}
type WriteFileTool struct {
workspace string
restrict bool
}
func NewWriteFileTool(workspace string, restrict bool) *WriteFileTool {
return &WriteFileTool{workspace: workspace, restrict: restrict}
}
func (t *WriteFileTool) Name() string {
return "write_file"
@@ -82,19 +130,31 @@ func (t *WriteFileTool) Execute(ctx context.Context, args map[string]interface{}
return ErrorResult("content is required")
}
dir := filepath.Dir(path)
resolvedPath, err := validatePath(path, t.workspace, t.restrict)
if err != nil {
return ErrorResult(err.Error())
}
dir := filepath.Dir(resolvedPath)
if err := os.MkdirAll(dir, 0755); err != nil {
return ErrorResult(fmt.Sprintf("failed to create directory: %v", err))
}
if err := os.WriteFile(path, []byte(content), 0644); err != nil {
if err := os.WriteFile(resolvedPath, []byte(content), 0644); err != nil {
return ErrorResult(fmt.Sprintf("failed to write file: %v", err))
}
return SilentResult(fmt.Sprintf("File written: %s", path))
}
type ListDirTool struct{}
type ListDirTool struct {
workspace string
restrict bool
}
func NewListDirTool(workspace string, restrict bool) *ListDirTool {
return &ListDirTool{workspace: workspace, restrict: restrict}
}
func (t *ListDirTool) Name() string {
return "list_dir"
@@ -123,7 +183,12 @@ func (t *ListDirTool) Execute(ctx context.Context, args map[string]interface{})
path = "."
}
entries, err := os.ReadDir(path)
resolvedPath, err := validatePath(path, t.workspace, t.restrict)
if err != nil {
return ErrorResult(err.Error())
}
entries, err := os.ReadDir(resolvedPath)
if err != nil {
return ErrorResult(fmt.Sprintf("failed to read directory: %v", err))
}

15
pkg/tools/shell.go
View File

@@ -8,10 +8,12 @@ import (
"os/exec"
"path/filepath"
"regexp"
"runtime"
"strings"
"time"
)
type ExecTool struct {
workingDir string
timeout time.Duration
@@ -20,14 +22,14 @@ type ExecTool struct {
restrictToWorkspace bool
}
func NewExecTool(workingDir string) *ExecTool {
func NewExecTool(workingDir string, restrict bool) *ExecTool {
denyPatterns := []*regexp.Regexp{
regexp.MustCompile(`\brm\s+-[rf]{1,2}\b`),
regexp.MustCompile(`\bdel\s+/[fq]\b`),
regexp.MustCompile(`\brmdir\s+/s\b`),
regexp.MustCompile(`\b(format|mkfs|diskpart)\b\s`), // Match disk wiping commands (must be followed by space/args)
regexp.MustCompile(`\bdd\s+if=`),
regexp.MustCompile(`>\s*/dev/sd[a-z]\b`), // Block writes to disk devices (but allow /dev/null)
regexp.MustCompile(`>\s*/dev/sd[a-z]\b`), // Block writes to disk devices (but allow /dev/null)
regexp.MustCompile(`\b(shutdown|reboot|poweroff)\b`),
regexp.MustCompile(`:\(\)\s*\{.*\};\s*:`),
}
@@ -37,7 +39,7 @@ func NewExecTool(workingDir string) *ExecTool {
timeout: 60 * time.Second,
denyPatterns: denyPatterns,
allowPatterns: nil,
restrictToWorkspace: false,
restrictToWorkspace: restrict,
}
}
@@ -91,7 +93,12 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]interface{}) *To
cmdCtx, cancel := context.WithTimeout(ctx, t.timeout)
defer cancel()
cmd := exec.CommandContext(cmdCtx, "sh", "-c", command)
var cmd *exec.Cmd
if runtime.GOOS == "windows" {
cmd = exec.CommandContext(cmdCtx, "powershell", "-NoProfile", "-NonInteractive", "-Command", command)
} else {
cmd = exec.CommandContext(cmdCtx, "sh", "-c", command)
}
if cwd != "" {
cmd.Dir = cwd
}

18
pkg/tools/shell_test.go
View File

@@ -11,7 +11,7 @@ import (
// TestShellTool_Success verifies successful command execution
func TestShellTool_Success(t *testing.T) {
tool := NewExecTool("")
tool := NewExecTool("", false)
ctx := context.Background()
args := map[string]interface{}{
@@ -38,7 +38,7 @@ func TestShellTool_Success(t *testing.T) {
// TestShellTool_Failure verifies failed command execution
func TestShellTool_Failure(t *testing.T) {
tool := NewExecTool("")
tool := NewExecTool("", false)
ctx := context.Background()
args := map[string]interface{}{
@@ -65,7 +65,7 @@ func TestShellTool_Failure(t *testing.T) {
// TestShellTool_Timeout verifies command timeout handling
func TestShellTool_Timeout(t *testing.T) {
tool := NewExecTool("")
tool := NewExecTool("", false)
tool.SetTimeout(100 * time.Millisecond)
ctx := context.Background()
@@ -93,7 +93,7 @@ func TestShellTool_WorkingDir(t *testing.T) {
testFile := filepath.Join(tmpDir, "test.txt")
os.WriteFile(testFile, []byte("test content"), 0644)
tool := NewExecTool("")
tool := NewExecTool("", false)
ctx := context.Background()
args := map[string]interface{}{
@@ -114,7 +114,7 @@ func TestShellTool_WorkingDir(t *testing.T) {
// TestShellTool_DangerousCommand verifies safety guard blocks dangerous commands
func TestShellTool_DangerousCommand(t *testing.T) {
tool := NewExecTool("")
tool := NewExecTool("", false)
ctx := context.Background()
args := map[string]interface{}{
@@ -135,7 +135,7 @@ func TestShellTool_DangerousCommand(t *testing.T) {
// TestShellTool_MissingCommand verifies error handling for missing command
func TestShellTool_MissingCommand(t *testing.T) {
tool := NewExecTool("")
tool := NewExecTool("", false)
ctx := context.Background()
args := map[string]interface{}{}
@@ -150,7 +150,7 @@ func TestShellTool_MissingCommand(t *testing.T) {
// TestShellTool_StderrCapture verifies stderr is captured and included
func TestShellTool_StderrCapture(t *testing.T) {
tool := NewExecTool("")
tool := NewExecTool("", false)
ctx := context.Background()
args := map[string]interface{}{
@@ -170,7 +170,7 @@ func TestShellTool_StderrCapture(t *testing.T) {
// TestShellTool_OutputTruncation verifies long output is truncated
func TestShellTool_OutputTruncation(t *testing.T) {
tool := NewExecTool("")
tool := NewExecTool("", false)
ctx := context.Background()
// Generate long output (>10000 chars)
@@ -189,7 +189,7 @@ func TestShellTool_OutputTruncation(t *testing.T) {
// TestShellTool_RestrictToWorkspace verifies workspace restriction
func TestShellTool_RestrictToWorkspace(t *testing.T) {
tmpDir := t.TempDir()
tool := NewExecTool(tmpDir)
tool := NewExecTool(tmpDir, false)
tool.SetRestrictToWorkspace(true)
ctx := context.Background()