From 00bf2f3786d2ea780ceffe74e1686d98a9648603 Mon Sep 17 00:00:00 2001 From: dj-oyu <68707227+dj-oyu@users.noreply.github.com> Date: Mon, 2 Mar 2026 01:46:22 +0900 Subject: [PATCH] refactor: remove unused workspace field from GitPushTool and CreatePRTool Both tools get their working directory from worktree context, making the workspace field redundant. Also replace custom contains helper with strings.Contains in tests. Co-Authored-By: Claude Opus 4.6 --- pkg/agent/instance.go | 4 ++-- pkg/tools/createpr.go | 10 +++++----- pkg/tools/createpr_test.go | 17 +++++++++-------- pkg/tools/gitpush.go | 8 +++----- pkg/tools/gitpush_test.go | 28 ++++++++-------------------- pkg/tools/subagent.go | 4 ++-- 6 files changed, 29 insertions(+), 42 deletions(-) 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