diff --git a/pkg/agent/instance.go b/pkg/agent/instance.go index a52024dd2..e0cf3ebf1 100644 --- a/pkg/agent/instance.go +++ b/pkg/agent/instance.go @@ -80,8 +80,8 @@ func NewAgentInstance( toolsRegistry.Register(tools.NewEditFileTool(workspace, restrict)) toolsRegistry.Register(tools.NewAppendFileTool(workspace, restrict)) toolsRegistry.Register(tools.NewLogsTool()) - toolsRegistry.Register(tools.NewGitPushTool(workspace)) - toolsRegistry.Register(tools.NewCreatePRTool(workspace)) + toolsRegistry.Register(tools.NewGitPushTool()) + toolsRegistry.Register(tools.NewCreatePRTool()) sessionsDir := filepath.Join(workspace, "sessions") sessionsManager := session.NewSessionManager(sessionsDir) diff --git a/pkg/tools/createpr.go b/pkg/tools/createpr.go index 4e5b652b6..c0f351fb3 100644 --- a/pkg/tools/createpr.go +++ b/pkg/tools/createpr.go @@ -27,13 +27,12 @@ const ( // - 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 { - workspace string - callback AsyncCallback + callback AsyncCallback } // NewCreatePRTool creates a CreatePRTool. -func NewCreatePRTool(workspace string) *CreatePRTool { - return &CreatePRTool{workspace: workspace} +func NewCreatePRTool() *CreatePRTool { + return &CreatePRTool{} } func (t *CreatePRTool) Name() string { return "create_pr" } @@ -151,7 +150,8 @@ func (t *CreatePRTool) Execute(ctx context.Context, args map[string]any) *ToolRe } // Build gh pr create command - ghArgs := []string{"pr", "create", + ghArgs := []string{ + "pr", "create", "--base", baseBranch, "--head", branch, "--title", title, diff --git a/pkg/tools/createpr_test.go b/pkg/tools/createpr_test.go index 0100d6171..26c330b7c 100644 --- a/pkg/tools/createpr_test.go +++ b/pkg/tools/createpr_test.go @@ -2,6 +2,7 @@ package tools import ( "context" + "strings" "testing" "github.com/sipeed/picoclaw/pkg/git" @@ -9,7 +10,7 @@ import ( // TestCreatePRTool_NoWorktree verifies that create_pr fails without worktree context. func TestCreatePRTool_NoWorktree(t *testing.T) { - tool := NewCreatePRTool(t.TempDir()) + tool := NewCreatePRTool() result := tool.Execute(context.Background(), map[string]any{ "title": "Test PR", @@ -23,7 +24,7 @@ func TestCreatePRTool_NoWorktree(t *testing.T) { // TestCreatePRTool_EmptyBranch verifies that empty branch name is rejected. func TestCreatePRTool_EmptyBranch(t *testing.T) { - tool := NewCreatePRTool(t.TempDir()) + tool := NewCreatePRTool() ctx := WithWorktreeInfo(context.Background(), &git.WorktreeInfo{ Branch: "", @@ -42,7 +43,7 @@ func TestCreatePRTool_EmptyBranch(t *testing.T) { // TestCreatePRTool_MissingTitle verifies that missing title is rejected. func TestCreatePRTool_MissingTitle(t *testing.T) { - tool := NewCreatePRTool(t.TempDir()) + tool := NewCreatePRTool() ctx := WithWorktreeInfo(context.Background(), &git.WorktreeInfo{ Branch: "plan/test", @@ -73,7 +74,7 @@ func TestCreatePRTool_MissingTitle(t *testing.T) { // TestCreatePRTool_BranchNotPushed verifies the tool checks for remote branch existence. func TestCreatePRTool_BranchNotPushed(t *testing.T) { - tool := NewCreatePRTool(t.TempDir()) + tool := NewCreatePRTool() ctx := WithWorktreeInfo(context.Background(), &git.WorktreeInfo{ Branch: "plan/not-pushed", @@ -93,7 +94,7 @@ func TestCreatePRTool_BranchNotPushed(t *testing.T) { // TestCreatePRTool_DefaultBaseBranch verifies fallback to "main" when BaseBranch is empty. func TestCreatePRTool_DefaultBaseBranch(t *testing.T) { - tool := NewCreatePRTool(t.TempDir()) + tool := NewCreatePRTool() // With empty BaseBranch, tool should default to "main" ctx := WithWorktreeInfo(context.Background(), &git.WorktreeInfo{ @@ -106,7 +107,7 @@ func TestCreatePRTool_DefaultBaseBranch(t *testing.T) { "title": "Test PR", }) // Will fail at ls-remote (no real repo), but should not fail at baseBranch validation - if result.IsError && contains(result.ForLLM, "base branch") { + if result.IsError && strings.Contains(result.ForLLM, "base branch") { t.Fatal("should not fail on base branch when defaulting to main") } } @@ -116,7 +117,7 @@ func TestCreatePRTool_Interface(t *testing.T) { var _ Tool = (*CreatePRTool)(nil) var _ AsyncTool = (*CreatePRTool)(nil) - tool := NewCreatePRTool(t.TempDir()) + tool := NewCreatePRTool() if tool.Name() != "create_pr" { t.Errorf("Name: got %q, want %q", tool.Name(), "create_pr") } @@ -146,7 +147,7 @@ func TestCreatePRTool_Interface(t *testing.T) { // TestCreatePRTool_SetCallback verifies callback is stored. func TestCreatePRTool_SetCallback(t *testing.T) { - tool := NewCreatePRTool(t.TempDir()) + tool := NewCreatePRTool() if tool.callback != nil { t.Fatal("callback should be nil initially") } diff --git a/pkg/tools/gitpush.go b/pkg/tools/gitpush.go index 0a91dce7f..3d8bc2b9b 100644 --- a/pkg/tools/gitpush.go +++ b/pkg/tools/gitpush.go @@ -38,13 +38,11 @@ var protectedBranches = regexp.MustCompile(`^(main|master|develop|release/.*)$`) // - Protected branches (main, master, develop, release/*) are blocked // - Force push is never allowed // - Auto-commits uncommitted changes before pushing -type GitPushTool struct { - workspace string -} +type GitPushTool struct{} // NewGitPushTool creates a GitPushTool. -func NewGitPushTool(workspace string) *GitPushTool { - return &GitPushTool{workspace: workspace} +func NewGitPushTool() *GitPushTool { + return &GitPushTool{} } func (t *GitPushTool) Name() string { return "git_push" } diff --git a/pkg/tools/gitpush_test.go b/pkg/tools/gitpush_test.go index 7842742a2..d16c446d4 100644 --- a/pkg/tools/gitpush_test.go +++ b/pkg/tools/gitpush_test.go @@ -2,6 +2,7 @@ package tools import ( "context" + "strings" "testing" "github.com/sipeed/picoclaw/pkg/git" @@ -9,7 +10,7 @@ import ( // TestGitPushTool_NoWorktree verifies that git_push fails without worktree context. func TestGitPushTool_NoWorktree(t *testing.T) { - tool := NewGitPushTool(t.TempDir()) + tool := NewGitPushTool() result := tool.Execute(context.Background(), map[string]any{}) if !result.IsError { @@ -25,7 +26,7 @@ func TestGitPushTool_NoWorktree(t *testing.T) { // TestGitPushTool_ProtectedBranch verifies that protected branches are blocked. func TestGitPushTool_ProtectedBranch(t *testing.T) { - tool := NewGitPushTool(t.TempDir()) + tool := NewGitPushTool() protectedNames := []string{"main", "master", "develop", "release/v1.0"} @@ -49,7 +50,7 @@ func TestGitPushTool_ProtectedBranch(t *testing.T) { // TestGitPushTool_EmptyBranch verifies that empty branch name is rejected. func TestGitPushTool_EmptyBranch(t *testing.T) { - tool := NewGitPushTool(t.TempDir()) + tool := NewGitPushTool() ctx := WithWorktreeInfo(context.Background(), &git.WorktreeInfo{ Branch: "", @@ -67,7 +68,7 @@ func TestGitPushTool_EmptyBranch(t *testing.T) { // 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(t.TempDir()) + tool := NewGitPushTool() allowedNames := []string{"plan/add-auth", "feature/foo", "worktree/test"} @@ -81,7 +82,7 @@ func TestGitPushTool_AllowedBranch(t *testing.T) { }) result := tool.Execute(ctx, map[string]any{}) // Should NOT fail with "protected branch" error - if result.IsError && contains(result.ForLLM, "protected") { + if result.IsError && strings.Contains(result.ForLLM, "protected") { t.Fatalf("branch %q should not be blocked as protected", branch) } }) @@ -147,7 +148,7 @@ func TestWorktreeInfoContext(t *testing.T) { func TestGitPushTool_Interface(t *testing.T) { var _ Tool = (*GitPushTool)(nil) - tool := NewGitPushTool(t.TempDir()) + tool := NewGitPushTool() if tool.Name() != "git_push" { t.Errorf("Name: got %q, want %q", tool.Name(), "git_push") } @@ -163,22 +164,9 @@ func TestGitPushTool_Interface(t *testing.T) { } } -func contains(s, substr string) bool { - return len(s) >= len(substr) && (s == substr || len(s) > 0 && containsStr(s, substr)) -} - -func containsStr(s, sub string) bool { - for i := 0; i+len(sub) <= len(s); i++ { - if s[i:i+len(sub)] == sub { - return true - } - } - return false -} - func assertContains(t *testing.T, s, substr string) { t.Helper() - if !contains(s, substr) { + if !strings.Contains(s, substr) { t.Errorf("expected %q to contain %q", s, substr) } } diff --git a/pkg/tools/subagent.go b/pkg/tools/subagent.go index 1599df454..157b40d4b 100644 --- a/pkg/tools/subagent.go +++ b/pkg/tools/subagent.go @@ -360,10 +360,10 @@ 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(sm.workspace)) + registry.Register(NewGitPushTool()) } if config.AllowedTools["create_pr"] { - registry.Register(NewCreatePRTool(sm.workspace)) + registry.Register(NewCreatePRTool()) } // Register web tools