diff --git a/pkg/agent/instance.go b/pkg/agent/instance.go index 1ba2faea1..e0cf3ebf1 100644 --- a/pkg/agent/instance.go +++ b/pkg/agent/instance.go @@ -80,6 +80,8 @@ func NewAgentInstance( toolsRegistry.Register(tools.NewEditFileTool(workspace, restrict)) toolsRegistry.Register(tools.NewAppendFileTool(workspace, restrict)) toolsRegistry.Register(tools.NewLogsTool()) + toolsRegistry.Register(tools.NewGitPushTool()) + toolsRegistry.Register(tools.NewCreatePRTool()) sessionsDir := filepath.Join(workspace, "sessions") sessionsManager := session.NewSessionManager(sessionsDir) diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index 0d8f71bb1..32e2584a8 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -2614,20 +2614,35 @@ func (al *AgentLoop) runLLMIteration( } } - // Create async callback for tools that implement AsyncTool - // NOTE: Following openclaw's design, async tools do NOT send results directly to users. - // Instead, they notify the agent via PublishInbound, and the agent decides - // whether to forward the result to the user (in processSystemMessage). + // Create async callback for tools that implement AsyncTool. + // The callback publishes a system inbound message so processSystemMessage + // injects the result into the conductor's session history. The conductor + // sees it on its next turn and decides whether to notify the user. + toolName := tc.Name // capture for goroutine asyncCallback := func(callbackCtx context.Context, result *tools.ToolResult) { - // Log the async completion but don't send directly to user - // The agent will handle user notification via processSystemMessage - if !result.Silent && result.ForUser != "" { - logger.InfoCF("agent", "Async tool completed, agent will handle notification", - map[string]any{ - "tool": tc.Name, - "content_len": len(result.ForUser), - }) + content := result.ForLLM + if content == "" { + content = result.ForUser } + if content == "" { + return + } + + logger.InfoCF("agent", "Async tool completed, publishing to conductor", + map[string]any{ + "tool": toolName, + "content_len": len(content), + "is_error": result.IsError, + }) + + pubCtx, pubCancel := context.WithTimeout(context.Background(), 5*time.Second) + defer pubCancel() + _ = al.bus.PublishInbound(pubCtx, bus.InboundMessage{ + Channel: "system", + SenderID: fmt.Sprintf("async:%s", toolName), + ChatID: fmt.Sprintf("%s:%s", opts.Channel, opts.ChatID), + Content: fmt.Sprintf("Async tool '%s' completed.\n\nResult:\n%s", toolName, content), + }) } // Report toolcall state to canvas. @@ -2637,6 +2652,7 @@ func (al *AgentLoop) runLLMIteration( toolCtx := ctx if wt := agent.GetWorktree(opts.SessionKey); wt != nil { toolCtx = tools.WithWorkspaceOverride(toolCtx, wt.Path) + toolCtx = tools.WithWorktreeInfo(toolCtx, wt) } toolResult := agent.Tools.ExecuteWithContext( toolCtx, diff --git a/pkg/tools/createpr.go b/pkg/tools/createpr.go new file mode 100644 index 000000000..c0f351fb3 --- /dev/null +++ b/pkg/tools/createpr.go @@ -0,0 +1,301 @@ +package tools + +import ( + "context" + "fmt" + "os/exec" + "strings" + "time" +) + +const ( + ciPollInterval = 30 * time.Second + ciPollTimeout = 15 * time.Minute +) + +// CreatePRTool creates a GitHub pull request from the current worktree branch. +// +// Safety invariants: +// - Only works inside a worktree (WorktreeInfo must be in context) +// - Base branch is auto-detected from WorktreeInfo.BaseBranch +// - Requires the branch to be already pushed (use git_push first) +// - Checks for merge conflicts with base before creating +// - Uses `gh pr create` under the hood +// +// Async behavior: +// - PR creation itself is synchronous and returns immediately with the PR URL +// - If CI runs are triggered, a background goroutine polls `gh pr checks` +// and calls the AsyncCallback when CI completes (pass or fail) +type CreatePRTool struct { + callback AsyncCallback +} + +// NewCreatePRTool creates a CreatePRTool. +func NewCreatePRTool() *CreatePRTool { + return &CreatePRTool{} +} + +func (t *CreatePRTool) Name() string { return "create_pr" } + +// SetCallback implements AsyncTool for CI completion notification. +func (t *CreatePRTool) SetCallback(cb AsyncCallback) { + t.callback = cb +} + +func (t *CreatePRTool) Description() string { + return "Create a GitHub pull request from the current worktree branch. " + + "The base branch is auto-detected from the worktree's parent branch. " + + "The branch must be pushed to origin first (use git_push). " + + "Checks for merge conflicts with the base branch before creating. " + + "After PR creation, polls CI status in the background and notifies when complete. " + + "Requires the `gh` CLI to be installed and authenticated." +} + +func (t *CreatePRTool) Parameters() map[string]any { + return map[string]any{ + "type": "object", + "properties": map[string]any{ + "title": map[string]any{ + "type": "string", + "description": "Pull request title", + }, + "body": map[string]any{ + "type": "string", + "description": "Pull request body/description (supports markdown)", + }, + "draft": map[string]any{ + "type": "boolean", + "description": "Create as draft PR (default: false)", + }, + }, + "required": []string{"title"}, + } +} + +func (t *CreatePRTool) Execute(ctx context.Context, args map[string]any) *ToolResult { + wt := WorktreeInfoFromCtx(ctx) + if wt == nil { + return ErrorResult( + "create_pr requires an active worktree.\n" + + "This tool can only be used during worktree-based sessions " + + "(e.g., heartbeat tasks or plan executing phase).\n" + + "The worktree provides the branch name and base branch for the PR.") + } + + branch := wt.Branch + if branch == "" { + return ErrorResult( + "worktree has no branch name.\n" + + "The WorktreeInfo was set but Branch is empty. " + + "This is an internal error — the worktree may not have been created correctly.") + } + + baseBranch := wt.BaseBranch + if baseBranch == "" { + baseBranch = "main" + } + + title, ok := args["title"].(string) + if !ok || strings.TrimSpace(title) == "" { + return ErrorResult( + "title is required.\n" + + "Provide a concise PR title describing the change (e.g., \"Add rate limiter to API endpoints\").") + } + + // Verify the branch has been pushed by checking if the remote ref exists + checkCtx, checkCancel := context.WithTimeout(ctx, 15*time.Second) + defer checkCancel() + checkCmd := exec.CommandContext(checkCtx, "git", "ls-remote", "--exit-code", "origin", branch) + checkCmd.Dir = wt.Path + if err := checkCmd.Run(); err != nil { + return ErrorResult(fmt.Sprintf( + "branch %q not found on origin.\n"+ + "The branch must be pushed before creating a PR. Use the git_push tool first.\n"+ + "git_push will auto-commit uncommitted changes and push the worktree branch to origin.", + branch)) + } + + // Fetch latest base branch and check for merge conflicts + fetchCtx, fetchCancel := context.WithTimeout(ctx, 30*time.Second) + defer fetchCancel() + fetchCmd := exec.CommandContext(fetchCtx, "git", "fetch", "origin", baseBranch) + fetchCmd.Dir = wt.Path + if out, err := fetchCmd.CombinedOutput(); err != nil { + return ErrorResult(fmt.Sprintf( + "failed to fetch origin/%s: %s\n%s\n"+ + "Cannot verify merge compatibility without the latest base branch. "+ + "Check network connectivity and that the base branch %q exists on origin.", + baseBranch, err, strings.TrimSpace(string(out)), baseBranch)) + } + + // Try a merge dry-run to detect conflicts. + // merge-tree --write-tree is a plumbing command (Git 2.38+) that performs a + // three-way merge entirely in-memory without touching the working tree. + // Exit code 0 = clean merge, non-zero = conflicts detected. + mergeCtx, mergeCancel := context.WithTimeout(ctx, 30*time.Second) + defer mergeCancel() + mergeCmd := exec.CommandContext(mergeCtx, "git", "merge-tree", + "--write-tree", "--no-messages", + branch, "origin/"+baseBranch) + mergeCmd.Dir = wt.RepoRoot + mergeOut, mergeErr := mergeCmd.CombinedOutput() + if mergeErr != nil { + conflictInfo := strings.TrimSpace(string(mergeOut)) + return ErrorResult(fmt.Sprintf( + "merge conflict detected between %q and %s.\n"+ + "The PR cannot be created cleanly. Resolve the conflicts in the worktree first, "+ + "then use git_push to push the resolution before retrying create_pr.\n"+ + "Conflict details:\n%s", + branch, baseBranch, conflictInfo)) + } + + // Build gh pr create command + ghArgs := []string{ + "pr", "create", + "--base", baseBranch, + "--head", branch, + "--title", title, + } + + if body, ok := args["body"].(string); ok && body != "" { + ghArgs = append(ghArgs, "--body", body) + } else { + ghArgs = append(ghArgs, "--body", "") + } + + if draft, ok := args["draft"].(bool); ok && draft { + ghArgs = append(ghArgs, "--draft") + } + + prCtx, prCancel := context.WithTimeout(ctx, 30*time.Second) + defer prCancel() + + cmd := exec.CommandContext(prCtx, "gh", ghArgs...) + cmd.Dir = wt.RepoRoot + out, err := cmd.CombinedOutput() + output := strings.TrimSpace(string(out)) + + if err != nil { + return ErrorResult(fmt.Sprintf( + "gh pr create failed: %s\n%s\n"+ + "Possible causes:\n"+ + "- gh CLI not installed or not authenticated (run `gh auth login`)\n"+ + "- A PR already exists for branch %q (check with `gh pr list`)\n"+ + "- Repository not configured as a GitHub remote", + err, output, branch)) + } + + prURL := output // gh pr create outputs the PR URL + + // Start background CI polling if callback is set + if t.callback != nil && prURL != "" { + cb := t.callback + repoRoot := wt.RepoRoot + go pollCIStatus(repoRoot, prURL, cb) + } + + return AsyncResult(fmt.Sprintf( + "Pull request created: %s\n"+ + "Branch: %s -> %s\n"+ + "CI status will be reported asynchronously when checks complete.", + prURL, branch, baseBranch)) +} + +// pollCIStatus polls `gh pr checks` in the background until all checks +// pass, fail, or the timeout is reached. Reports back via AsyncCallback. +func pollCIStatus(repoRoot, prURL string, callback AsyncCallback) { + // Detached context with hard timeout — this goroutine outlives the tool call. + ctx, cancel := context.WithTimeout(context.Background(), ciPollTimeout) + defer cancel() + + // Initial wait: CI runs take a few seconds to register after PR creation + select { + case <-time.After(10 * time.Second): + case <-ctx.Done(): + return + } + + ticker := time.NewTicker(ciPollInterval) + defer ticker.Stop() + + for { + status, detail := checkPRChecks(ctx, repoRoot, prURL) + switch status { + case ciStatusPass: + callback(ctx, NewToolResult(fmt.Sprintf( + "CI passed for %s\n%s", + prURL, detail))) + return + case ciStatusFail: + callback(ctx, &ToolResult{ + ForLLM: fmt.Sprintf( + "CI failed for %s\n%s\n"+ + "Run `gh run view` for detailed logs.", + prURL, detail), + IsError: true, + }) + return + case ciStatusNone: + callback(ctx, NewToolResult(fmt.Sprintf( + "No CI checks configured for %s. PR is ready for review.", + prURL))) + return + case ciStatusPending: + // Still running, continue polling + } + + select { + case <-ticker.C: + case <-ctx.Done(): + callback(ctx, &ToolResult{ + ForLLM: fmt.Sprintf( + "CI polling timed out after %s for %s.\n"+ + "Checks may still be running. Run `gh pr checks %s` to check.", + ciPollTimeout, prURL, prURL), + IsError: true, + }) + return + } + } +} + +type ciStatus int + +const ( + ciStatusPending ciStatus = iota + ciStatusPass + ciStatusFail + ciStatusNone +) + +// checkPRChecks runs `gh pr checks` and parses the result. +// Returns the aggregate status and raw output for the caller to include. +func checkPRChecks(ctx context.Context, repoRoot, prURL string) (ciStatus, string) { + checkCtx, cancel := context.WithTimeout(ctx, 15*time.Second) + defer cancel() + + cmd := exec.CommandContext(checkCtx, "gh", "pr", "checks", prURL) + cmd.Dir = repoRoot + out, err := cmd.CombinedOutput() + output := strings.TrimSpace(string(out)) + + if err != nil { + // gh pr checks exits 1 when any check has failed + if strings.Contains(output, "fail") || strings.Contains(output, "X ") { + return ciStatusFail, output + } + // "no checks" case + if strings.Contains(output, "no checks") || output == "" { + return ciStatusNone, "" + } + // Transient error or still pending — keep polling + return ciStatusPending, output + } + + // Exit 0: all checks completed. Check for pending. + if strings.Contains(output, "pending") || strings.Contains(output, "- ") { + return ciStatusPending, output + } + + return ciStatusPass, output +} diff --git a/pkg/tools/createpr_test.go b/pkg/tools/createpr_test.go new file mode 100644 index 000000000..26c330b7c --- /dev/null +++ b/pkg/tools/createpr_test.go @@ -0,0 +1,212 @@ +package tools + +import ( + "context" + "strings" + "testing" + + "github.com/sipeed/picoclaw/pkg/git" +) + +// TestCreatePRTool_NoWorktree verifies that create_pr fails without worktree context. +func TestCreatePRTool_NoWorktree(t *testing.T) { + tool := NewCreatePRTool() + + result := tool.Execute(context.Background(), map[string]any{ + "title": "Test PR", + }) + if !result.IsError { + t.Fatal("expected error when no worktree in context") + } + assertContains(t, result.ForLLM, "worktree") + assertContains(t, result.ForLLM, "heartbeat") +} + +// TestCreatePRTool_EmptyBranch verifies that empty branch name is rejected. +func TestCreatePRTool_EmptyBranch(t *testing.T) { + tool := NewCreatePRTool() + + ctx := WithWorktreeInfo(context.Background(), &git.WorktreeInfo{ + Branch: "", + BaseBranch: "main", + Path: t.TempDir(), + RepoRoot: t.TempDir(), + }) + result := tool.Execute(ctx, map[string]any{ + "title": "Test PR", + }) + if !result.IsError { + t.Fatal("expected error for empty branch") + } + assertContains(t, result.ForLLM, "no branch name") +} + +// TestCreatePRTool_MissingTitle verifies that missing title is rejected. +func TestCreatePRTool_MissingTitle(t *testing.T) { + tool := NewCreatePRTool() + + ctx := WithWorktreeInfo(context.Background(), &git.WorktreeInfo{ + Branch: "plan/test", + BaseBranch: "main", + Path: t.TempDir(), + RepoRoot: t.TempDir(), + }) + + tests := []struct { + name string + args map[string]any + }{ + {"no title key", map[string]any{}}, + {"empty title", map[string]any{"title": ""}}, + {"whitespace title", map[string]any{"title": " "}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := tool.Execute(ctx, tt.args) + if !result.IsError { + t.Fatal("expected error for missing/empty title") + } + assertContains(t, result.ForLLM, "title is required") + }) + } +} + +// TestCreatePRTool_BranchNotPushed verifies the tool checks for remote branch existence. +func TestCreatePRTool_BranchNotPushed(t *testing.T) { + tool := NewCreatePRTool() + + ctx := WithWorktreeInfo(context.Background(), &git.WorktreeInfo{ + Branch: "plan/not-pushed", + BaseBranch: "main", + Path: t.TempDir(), + RepoRoot: t.TempDir(), + }) + result := tool.Execute(ctx, map[string]any{ + "title": "Test PR", + }) + if !result.IsError { + t.Fatal("expected error for unpushed branch") + } + // Should mention git_push as the remedy + assertContains(t, result.ForLLM, "git_push") +} + +// TestCreatePRTool_DefaultBaseBranch verifies fallback to "main" when BaseBranch is empty. +func TestCreatePRTool_DefaultBaseBranch(t *testing.T) { + tool := NewCreatePRTool() + + // With empty BaseBranch, tool should default to "main" + ctx := WithWorktreeInfo(context.Background(), &git.WorktreeInfo{ + Branch: "plan/test", + BaseBranch: "", + Path: t.TempDir(), + RepoRoot: t.TempDir(), + }) + result := tool.Execute(ctx, map[string]any{ + "title": "Test PR", + }) + // Will fail at ls-remote (no real repo), but should not fail at baseBranch validation + if result.IsError && strings.Contains(result.ForLLM, "base branch") { + t.Fatal("should not fail on base branch when defaulting to main") + } +} + +// TestCreatePRTool_Interface verifies the tool satisfies both Tool and AsyncTool interfaces. +func TestCreatePRTool_Interface(t *testing.T) { + var _ Tool = (*CreatePRTool)(nil) + var _ AsyncTool = (*CreatePRTool)(nil) + + tool := NewCreatePRTool() + if tool.Name() != "create_pr" { + t.Errorf("Name: got %q, want %q", tool.Name(), "create_pr") + } + if tool.Description() == "" { + t.Error("Description should not be empty") + } + params := tool.Parameters() + if params == nil { + t.Fatal("Parameters should not be nil") + } + + // Verify "title" is required + required, ok := params["required"].([]string) + if !ok { + t.Fatal("required should be []string") + } + foundTitle := false + for _, r := range required { + if r == "title" { + foundTitle = true + } + } + if !foundTitle { + t.Error("title should be in required parameters") + } +} + +// TestCreatePRTool_SetCallback verifies callback is stored. +func TestCreatePRTool_SetCallback(t *testing.T) { + tool := NewCreatePRTool() + if tool.callback != nil { + t.Fatal("callback should be nil initially") + } + + called := false + tool.SetCallback(func(ctx context.Context, result *ToolResult) { + called = true + }) + if tool.callback == nil { + t.Fatal("callback should be set after SetCallback") + } + // Verify it's callable (doesn't panic) + tool.callback(context.Background(), NewToolResult("test")) + if !called { + t.Fatal("callback was not invoked") + } +} + +// TestCheckPRChecks_ParseResults tests CI status parsing logic. +func TestCheckPRChecks_ParseResults(t *testing.T) { + // This tests the parsing logic conceptually — actual `gh` calls + // would need integration tests. We verify the status constants exist + // and the type is usable. + if ciStatusPending != 0 { + t.Error("ciStatusPending should be 0 (default)") + } + if ciStatusPass == ciStatusFail { + t.Error("ciStatusPass and ciStatusFail should differ") + } + if ciStatusNone == ciStatusPending { + t.Error("ciStatusNone and ciStatusPending should differ") + } +} + +// TestAllowedToolsForPreset_GitTools checks git tools are correctly assigned to presets. +func TestAllowedToolsForPreset_GitTools(t *testing.T) { + tests := []struct { + name string + preset Preset + wantGitPush bool + wantCreatePR bool + }{ + {"scout", PresetScout, false, false}, + {"analyst", PresetAnalyst, false, false}, + {"coder", PresetCoder, true, false}, + {"worker", PresetWorker, true, true}, + {"coordinator", PresetCoordinator, true, true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + allowed := AllowedToolsForPreset(tt.preset) + + if got := allowed["git_push"]; got != tt.wantGitPush { + t.Errorf("git_push: got %v, want %v", got, tt.wantGitPush) + } + if got := allowed["create_pr"]; got != tt.wantCreatePR { + t.Errorf("create_pr: got %v, want %v", got, tt.wantCreatePR) + } + }) + } +} diff --git a/pkg/tools/gitpush.go b/pkg/tools/gitpush.go new file mode 100644 index 000000000..3d8bc2b9b --- /dev/null +++ b/pkg/tools/gitpush.go @@ -0,0 +1,142 @@ +package tools + +import ( + "context" + "fmt" + "os/exec" + "regexp" + "strings" + "time" + + "github.com/sipeed/picoclaw/pkg/git" +) + +// worktreeInfoKey is the context key for passing WorktreeInfo to tools. +type worktreeInfoKey struct{} + +// WithWorktreeInfo returns a context carrying the active WorktreeInfo. +func WithWorktreeInfo(ctx context.Context, wt *git.WorktreeInfo) context.Context { + return context.WithValue(ctx, worktreeInfoKey{}, wt) +} + +// WorktreeInfoFromCtx extracts the WorktreeInfo from context, or nil. +func WorktreeInfoFromCtx(ctx context.Context) *git.WorktreeInfo { + if v, ok := ctx.Value(worktreeInfoKey{}).(*git.WorktreeInfo); ok { + return v + } + return nil +} + +// protectedBranches are branch names that can never be pushed to. +var protectedBranches = regexp.MustCompile(`^(main|master|develop|release/.*)$`) + +// GitPushTool implements safe git push restricted to worktree branches. +// +// Safety invariants: +// - Only works inside a worktree (WorktreeInfo must be in context) +// - Pushes only the worktree's branch — no arbitrary branch targets +// - Protected branches (main, master, develop, release/*) are blocked +// - Force push is never allowed +// - Auto-commits uncommitted changes before pushing +type GitPushTool struct{} + +// NewGitPushTool creates a GitPushTool. +func NewGitPushTool() *GitPushTool { + return &GitPushTool{} +} + +func (t *GitPushTool) Name() string { return "git_push" } + +func (t *GitPushTool) Description() string { + return "Push the current worktree branch to origin. Only works inside a git worktree. " + + "Auto-commits uncommitted changes before pushing. " + + "Protected branches (main, master, develop) cannot be pushed to. Force push is not allowed." +} + +func (t *GitPushTool) Parameters() map[string]any { + return map[string]any{ + "type": "object", + "properties": map[string]any{ + "commit_message": map[string]any{ + "type": "string", + "description": "Commit message for uncommitted changes. If omitted, uncommitted changes are auto-committed with a default message.", + }, + }, + "required": []string{}, + } +} + +func (t *GitPushTool) Execute(ctx context.Context, args map[string]any) *ToolResult { + wt := WorktreeInfoFromCtx(ctx) + if wt == nil { + return ErrorResult( + "git_push requires an active worktree.\n" + + "This tool can only be used during worktree-based sessions " + + "(e.g., heartbeat tasks or plan executing phase).\n" + + "The worktree provides the branch name and isolation boundary — " + + "without it, git_push cannot determine which branch to push.") + } + + branch := wt.Branch + if branch == "" { + return ErrorResult( + "worktree has no branch name.\n" + + "The WorktreeInfo was set but Branch is empty. " + + "This is an internal error — the worktree may not have been created correctly.") + } + + // Block protected branches + if protectedBranches.MatchString(branch) { + return ErrorResult(fmt.Sprintf( + "cannot push to protected branch %q.\n"+ + "Protected branches (main, master, develop, release/*) are blocked to prevent "+ + "accidental overwrites. Work should be done on feature branches created by worktrees.", + branch)) + } + + // Auto-commit uncommitted changes + if git.HasUncommittedChanges(wt.Path) { + commitMsg := "auto: save before push" + if msg, ok := args["commit_message"].(string); ok && msg != "" { + commitMsg = msg + } + if err := git.AutoCommit(wt.Path, commitMsg); err != nil { + return ErrorResult(fmt.Sprintf( + "auto-commit failed before push: %v\n"+ + "git_push auto-commits uncommitted changes before pushing. "+ + "The commit failed, so no push was attempted. "+ + "Check if the worktree at %q is in a valid state (e.g., no merge conflicts).", + err, wt.Path)) + } + } + + // Check there are commits to push + ahead := git.CommitsAhead(wt.RepoRoot, wt.BaseBranch, branch) + if ahead == 0 { + return NewToolResult(fmt.Sprintf( + "Nothing to push: branch %q has no commits ahead of %s.\n"+ + "The branch is identical to the base. Make changes and commit before pushing.", + branch, wt.BaseBranch)) + } + + // Push with -u (set upstream tracking) + pushCtx, cancel := context.WithTimeout(ctx, 2*time.Minute) + defer cancel() + + cmd := exec.CommandContext(pushCtx, "git", "push", "-u", "origin", branch) + cmd.Dir = wt.Path + out, err := cmd.CombinedOutput() + output := strings.TrimSpace(string(out)) + + if err != nil { + return ErrorResult(fmt.Sprintf( + "git push failed for branch %q: %s\n%s\n"+ + "Possible causes: network error, authentication failure, or remote rejected the push. "+ + "If the remote branch has diverged, resolve the divergence in the worktree first — "+ + "force push is not available.", + branch, err, output)) + } + + return NewToolResult(fmt.Sprintf("Pushed branch %q to origin (%d commit(s) ahead of %s)\n%s", + branch, ahead, wt.BaseBranch, output)) +} diff --git a/pkg/tools/gitpush_test.go b/pkg/tools/gitpush_test.go new file mode 100644 index 000000000..d16c446d4 --- /dev/null +++ b/pkg/tools/gitpush_test.go @@ -0,0 +1,172 @@ +package tools + +import ( + "context" + "strings" + "testing" + + "github.com/sipeed/picoclaw/pkg/git" +) + +// TestGitPushTool_NoWorktree verifies that git_push fails without worktree context. +func TestGitPushTool_NoWorktree(t *testing.T) { + tool := NewGitPushTool() + + result := tool.Execute(context.Background(), map[string]any{}) + if !result.IsError { + t.Fatal("expected error when no worktree in context") + } + if result.ForLLM == "" { + t.Fatal("error message should not be empty") + } + // Verify helpful guidance is included + assertContains(t, result.ForLLM, "worktree") + assertContains(t, result.ForLLM, "heartbeat") +} + +// TestGitPushTool_ProtectedBranch verifies that protected branches are blocked. +func TestGitPushTool_ProtectedBranch(t *testing.T) { + tool := NewGitPushTool() + + protectedNames := []string{"main", "master", "develop", "release/v1.0"} + + for _, branch := range protectedNames { + t.Run(branch, func(t *testing.T) { + ctx := WithWorktreeInfo(context.Background(), &git.WorktreeInfo{ + Branch: branch, + BaseBranch: "main", + Path: t.TempDir(), + RepoRoot: t.TempDir(), + }) + result := tool.Execute(ctx, map[string]any{}) + if !result.IsError { + t.Fatalf("expected error for protected branch %q", branch) + } + assertContains(t, result.ForLLM, "protected") + assertContains(t, result.ForLLM, branch) + }) + } +} + +// TestGitPushTool_EmptyBranch verifies that empty branch name is rejected. +func TestGitPushTool_EmptyBranch(t *testing.T) { + tool := NewGitPushTool() + + ctx := WithWorktreeInfo(context.Background(), &git.WorktreeInfo{ + Branch: "", + BaseBranch: "main", + Path: t.TempDir(), + RepoRoot: t.TempDir(), + }) + result := tool.Execute(ctx, map[string]any{}) + if !result.IsError { + t.Fatal("expected error for empty branch") + } + assertContains(t, result.ForLLM, "no branch name") +} + +// TestGitPushTool_AllowedBranch verifies that non-protected branches pass the branch check. +// (Push itself will fail because there's no real git repo, but it should get past validation.) +func TestGitPushTool_AllowedBranch(t *testing.T) { + tool := NewGitPushTool() + + allowedNames := []string{"plan/add-auth", "feature/foo", "worktree/test"} + + for _, branch := range allowedNames { + t.Run(branch, func(t *testing.T) { + ctx := WithWorktreeInfo(context.Background(), &git.WorktreeInfo{ + Branch: branch, + BaseBranch: "main", + Path: t.TempDir(), + RepoRoot: t.TempDir(), + }) + result := tool.Execute(ctx, map[string]any{}) + // Should NOT fail with "protected branch" error + if result.IsError && strings.Contains(result.ForLLM, "protected") { + t.Fatalf("branch %q should not be blocked as protected", branch) + } + }) + } +} + +// TestProtectedBranchesRegex tests the regex directly. +func TestProtectedBranchesRegex(t *testing.T) { + tests := []struct { + branch string + protected bool + }{ + {"main", true}, + {"master", true}, + {"develop", true}, + {"release/v1.0", true}, + {"release/2026-03", true}, + {"plan/add-feature", false}, + {"feature/main", false}, // "main" not at start + {"main-backup", false}, // "main" followed by suffix + {"hotfix/urgent", false}, + } + + for _, tt := range tests { + t.Run(tt.branch, func(t *testing.T) { + got := protectedBranches.MatchString(tt.branch) + if got != tt.protected { + t.Errorf("branch %q: got protected=%v, want %v", tt.branch, got, tt.protected) + } + }) + } +} + +// TestWorktreeInfoContext verifies context round-trip. +func TestWorktreeInfoContext(t *testing.T) { + wt := &git.WorktreeInfo{ + Branch: "plan/test", + BaseBranch: "main", + Path: "/tmp/wt", + RepoRoot: "/tmp/repo", + } + + ctx := WithWorktreeInfo(context.Background(), wt) + got := WorktreeInfoFromCtx(ctx) + if got == nil { + t.Fatal("expected non-nil WorktreeInfo from context") + } + if got.Branch != wt.Branch { + t.Errorf("Branch: got %q, want %q", got.Branch, wt.Branch) + } + if got.BaseBranch != wt.BaseBranch { + t.Errorf("BaseBranch: got %q, want %q", got.BaseBranch, wt.BaseBranch) + } + + // Nil case + got2 := WorktreeInfoFromCtx(context.Background()) + if got2 != nil { + t.Errorf("expected nil WorktreeInfo from bare context, got %+v", got2) + } +} + +// TestGitPushTool_Interface verifies the tool satisfies the Tool interface. +func TestGitPushTool_Interface(t *testing.T) { + var _ Tool = (*GitPushTool)(nil) + + tool := NewGitPushTool() + if tool.Name() != "git_push" { + t.Errorf("Name: got %q, want %q", tool.Name(), "git_push") + } + if tool.Description() == "" { + t.Error("Description should not be empty") + } + params := tool.Parameters() + if params == nil { + t.Fatal("Parameters should not be nil") + } + if params["type"] != "object" { + t.Errorf("Parameters type: got %v, want object", params["type"]) + } +} + +func assertContains(t *testing.T, s, substr string) { + t.Helper() + if !strings.Contains(s, substr) { + t.Errorf("expected %q to contain %q", s, substr) + } +} diff --git a/pkg/tools/sandbox.go b/pkg/tools/sandbox.go index 4a6306393..6e728cf1a 100644 --- a/pkg/tools/sandbox.go +++ b/pkg/tools/sandbox.go @@ -108,12 +108,18 @@ func AllowedToolsForPreset(p Preset) map[string]bool { allowed["exec"] = true } - // Add coder/worker/coordinator tools (write, bg_monitor) + // Add coder/worker/coordinator tools (write, bg_monitor, git_push) if p == PresetCoder || p == PresetWorker || p == PresetCoordinator { allowed["write_file"] = true allowed["edit_file"] = true allowed["append_file"] = true allowed["bg_monitor"] = true + allowed["git_push"] = true + } + + // Add worker/coordinator tools (create_pr) + if p == PresetWorker || p == PresetCoordinator { + allowed["create_pr"] = true } // Add coordinator-only tools (spawn) diff --git a/pkg/tools/subagent.go b/pkg/tools/subagent.go index beecb20b4..157b40d4b 100644 --- a/pkg/tools/subagent.go +++ b/pkg/tools/subagent.go @@ -358,6 +358,14 @@ func (sm *SubagentManager) buildPresetRegistry(preset Preset, writeRoot string) } } + // Register git tools (worktree-safe push and PR creation) + if config.AllowedTools["git_push"] { + registry.Register(NewGitPushTool()) + } + if config.AllowedTools["create_pr"] { + registry.Register(NewCreatePRTool()) + } + // Register web tools if config.AllowedTools["web_search"] { webSearchTool := NewWebSearchTool(sm.webSearchOpts)