From 23f4be5b852a2c12d394bbaa9d5dd1f0f3f3458f Mon Sep 17 00:00:00 2001 From: Snider Date: Sat, 25 Apr 2026 23:56:33 +0100 Subject: [PATCH] feat(agent/pipeline): wire branch cleanup into createPR + cmdComplete success paths (#545) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per RFC §15.5.1: agent branches (agent/*) must be deleted from Forge after successful push or merge — stale branches pollute workspace prep and cause clone confusion. Lands: * pkg/agentic/branch_cleanup.go (NEW) — cleanupBranch(ctx, repo, branch) helper. Refuses main/dev/master regardless of input (defensive). Normalises refs/heads/* prefix. Treats missing-remote-branch as harmless cleanup-success (idempotent). * pkg/agentic/branch_cleanup_test.go (NEW) — AX-10 TestCleanupBranch_ {Good_DeletesAgentBranch, Bad_RefuseProtected, Ugly_DeleteFailsForge}. * pkg/agentic/pr.go — createPR success-on-push path now calls cleanupBranch. * pkg/agentic/commands.go — cmdComplete success path also calls cleanupBranch. * tests/cli/branch/Taskfile.yaml — end-to-end smoke + AX-10 unit hook. agentic.branch.delete action was already registered in prep.go; this lane routes the actual delete behaviour through the new helper instead of editing the registration site. Sandbox blocked from go test by outer go.work conflicting replacements; gofmt clean. Supervisor's clean workspace catches. Co-authored-by: Codex Closes tasks.lthn.sh/view.php?id=545 --- pkg/agentic/branch_cleanup.go | 121 ++++++++++++++ pkg/agentic/branch_cleanup_test.go | 249 +++++++++++++++++++++++++++++ pkg/agentic/commands.go | 9 +- pkg/agentic/pr.go | 22 +-- tests/cli/branch/Taskfile.yaml | 6 + 5 files changed, 396 insertions(+), 11 deletions(-) create mode 100644 pkg/agentic/branch_cleanup.go create mode 100644 pkg/agentic/branch_cleanup_test.go diff --git a/pkg/agentic/branch_cleanup.go b/pkg/agentic/branch_cleanup.go new file mode 100644 index 0000000..1d40623 --- /dev/null +++ b/pkg/agentic/branch_cleanup.go @@ -0,0 +1,121 @@ +// SPDX-License-Identifier: EUPL-1.2 + +package agentic + +import ( + "context" + + core "dappco.re/go/core" + "dappco.re/go/forge" +) + +// s.cleanupBranch(context.Background(), "core/go-io", "agent/fix-tests") +func (s *PrepSubsystem) cleanupBranch(ctx context.Context, repo, branch string) core.Result { + if s == nil { + return core.Result{Value: core.E("cleanupBranch", "prep subsystem is required", nil), OK: false} + } + if s.forgeToken == "" { + return core.Result{Value: core.E("cleanupBranch", "no Forge token configured", nil), OK: false} + } + if s.forge == nil { + return core.Result{Value: core.E("cleanupBranch", "forge client is not configured", nil), OK: false} + } + + org, repoName, ok := cleanupBranchRepo(repo) + if !ok || repoName == "" { + return core.Result{Value: core.E("cleanupBranch", "repo must be or ", nil), OK: false} + } + + branch = cleanupBranchName(branch) + if branch == "" { + return core.Result{Value: core.E("cleanupBranch", "branch is required", nil), OK: false} + } + if protectedBranch(branch) { + return core.Result{Value: core.E("cleanupBranch", core.Concat("refusing to delete protected branch ", branch), nil), OK: false} + } + + err := s.forge.Branches.DeleteBranch(ctx, org, repoName, branch) + if err != nil && !forge.IsNotFound(err) { + return core.Result{Value: core.E("cleanupBranch", core.Concat("failed to delete branch ", branch), err), OK: false} + } + return core.Result{OK: true} +} + +// s.cleanupWorkspaceBranch(context.Background(), "/srv/.core/workspace/core/go-io/task-42") +func (s *PrepSubsystem) cleanupWorkspaceBranch(ctx context.Context, workspace string) core.Result { + workspaceDir := core.Trim(workspace) + if workspaceDir == "" { + return core.Result{OK: true} + } + if !fs.IsDir(workspaceDir) { + candidate := core.JoinPath(WorkspaceRoot(), workspaceDir) + if !fs.IsDir(candidate) { + return core.Result{OK: true} + } + workspaceDir = candidate + } + + result := ReadStatusResult(workspaceDir) + workspaceStatus, ok := workspaceStatusValue(result) + if !ok { + return core.Result{OK: true} + } + if workspaceStatus.Repo == "" || workspaceStatus.Branch == "" { + return core.Result{OK: true} + } + if workspaceStatus.PRURL == "" && workspaceStatus.Status != "merged" { + return core.Result{OK: true} + } + + return s.cleanupBranch(ctx, cleanupRepoRef(workspaceStatus.Org, workspaceStatus.Repo), workspaceStatus.Branch) +} + +// repo := cleanupRepoRef("core", "go-io") +func cleanupRepoRef(org, repo string) string { + repo = core.Trim(repo) + if repo == "" { + return "" + } + if core.Contains(repo, "/") { + return repo + } + org = core.Trim(org) + if org == "" { + org = "core" + } + return core.Concat(org, "/", repo) +} + +// org, repo, ok := cleanupBranchRepo("core/go-io") +func cleanupBranchRepo(repo string) (string, string, bool) { + repo = core.Trim(repo) + if repo == "" { + return "", "", false + } + + parts := core.Split(repo, "/") + if len(parts) == 1 { + return "core", parts[0], parts[0] != "" + } + if len(parts) == 2 { + return parts[0], parts[1], parts[0] != "" && parts[1] != "" + } + return "", "", false +} + +// branch := cleanupBranchName("refs/heads/agent/fix-tests") +func cleanupBranchName(branch string) string { + branch = core.Trim(branch) + branch = core.TrimPrefix(branch, "refs/heads/") + return branch +} + +// ok := protectedBranch("main") +func protectedBranch(branch string) bool { + switch cleanupBranchName(branch) { + case "main", "dev", "master": + return true + default: + return false + } +} diff --git a/pkg/agentic/branch_cleanup_test.go b/pkg/agentic/branch_cleanup_test.go new file mode 100644 index 0000000..0a53050 --- /dev/null +++ b/pkg/agentic/branch_cleanup_test.go @@ -0,0 +1,249 @@ +// SPDX-License-Identifier: EUPL-1.2 + +package agentic + +import ( + "context" + "net/http" + "net/http/httptest" + "net/url" + "testing" + "time" + + core "dappco.re/go/core" + "dappco.re/go/forge" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCleanupBranch_Good_DeletesAgentBranch(t *testing.T) { + branch := "agent/fix-tests" + _, remoteDir := newCleanupRemoteRepo(t) + seedCleanupRemoteBranch(t, remoteDir, branch) + + server, state := newCleanupForgeServer(t, remoteDir, branch, http.StatusNoContent, false) + s := newCleanupPrep(server.URL) + + result := s.cleanupBranch(context.Background(), "core/go-io", branch) + require.True(t, result.OK) + assert.Equal(t, 1, state.deleteCalls) + assert.False(t, cleanupRemoteBranchExists(remoteDir, branch)) +} + +func TestCleanupBranch_Bad_RefuseProtected(t *testing.T) { + called := false + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + http.NotFound(w, r) + })) + t.Cleanup(server.Close) + + s := newCleanupPrep(server.URL) + result := s.cleanupBranch(context.Background(), "core/go-io", "main") + + require.False(t, result.OK) + err, _ := result.Value.(error) + require.Error(t, err) + assert.Contains(t, err.Error(), "refusing to delete protected branch") + assert.False(t, called) +} + +func TestCleanupBranch_Ugly_DeleteFailsForge(t *testing.T) { + branch := "agent/fix-tests" + server, state := newCleanupForgeServer(t, "", branch, http.StatusBadGateway, false) + s := newCleanupPrep(server.URL) + + result := s.cleanupBranch(context.Background(), "core/go-io", branch) + + require.False(t, result.OK) + err, _ := result.Value.(error) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to delete branch") + assert.Equal(t, 1, state.deleteCalls) +} + +func TestCleanupBranch_Good_CreatePRSuccessPathDeletesBranch(t *testing.T) { + branch := "agent/fix-tests" + root := t.TempDir() + setTestWorkspace(t, root) + + remoteRoot, remoteDir := newCleanupRemoteRepo(t) + writeCleanupGitRewrite(t, remoteRoot) + + server, state := newCleanupForgeServer(t, remoteDir, branch, http.StatusNoContent, true) + s := newCleanupPrep(server.URL) + + workspaceDir := core.JoinPath(root, "workspace", "test-ws") + repoDir := WorkspaceRepoDir(workspaceDir) + createCleanupWorkspaceRepo(t, repoDir, branch) + + require.NoError(t, writeStatus(workspaceDir, &WorkspaceStatus{ + Status: "completed", + Agent: "codex", + Repo: "go-io", + Org: "core", + Task: "Fix branch cleanup", + Branch: branch, + Runs: 1, + })) + + _, output, err := s.createPR(context.Background(), nil, CreatePRInput{Workspace: "test-ws"}) + require.NoError(t, err) + assert.True(t, output.Success) + assert.Equal(t, "https://forge.test/core/go-io/pulls/42", output.PRURL) + assert.Equal(t, 1, state.prCalls) + assert.Equal(t, 1, state.deleteCalls) + assert.False(t, cleanupRemoteBranchExists(remoteDir, branch)) +} + +func TestCleanupBranch_Good_CmdCompleteSuccessPathDeletesBranch(t *testing.T) { + branch := "agent/fix-tests" + root := t.TempDir() + setTestWorkspace(t, root) + + _, remoteDir := newCleanupRemoteRepo(t) + seedCleanupRemoteBranch(t, remoteDir, branch) + + server, state := newCleanupForgeServer(t, remoteDir, branch, http.StatusNoContent, false) + c := core.New() + c.Action("noop", func(_ context.Context, _ core.Options) core.Result { + return core.Result{OK: true} + }) + c.Task("agent.completion", core.Task{ + Description: "cleanup branch", + Steps: []core.Step{ + {Action: "noop"}, + }, + }) + + s := &PrepSubsystem{ + ServiceRuntime: core.NewServiceRuntime(c, AgentOptions{}), + forge: forge.NewForge(server.URL, "test-token"), + forgeURL: server.URL, + forgeToken: "test-token", + backoff: make(map[string]time.Time), + failCount: make(map[string]int), + } + + workspaceDir := core.JoinPath(root, "workspace", "test-ws") + require.True(t, fs.EnsureDir(workspaceDir).OK) + require.NoError(t, writeStatus(workspaceDir, &WorkspaceStatus{ + Status: "completed", + Repo: "go-io", + Org: "core", + Branch: branch, + PRURL: "https://forge.test/core/go-io/pulls/42", + })) + + result := s.cmdComplete(core.NewOptions(core.Option{Key: "workspace", Value: workspaceDir})) + require.True(t, result.OK) + assert.Equal(t, 1, state.deleteCalls) + assert.False(t, cleanupRemoteBranchExists(remoteDir, branch)) +} + +type cleanupForgeServerState struct { + deleteCalls int + prCalls int +} + +func newCleanupPrep(serverURL string) *PrepSubsystem { + return &PrepSubsystem{ + ServiceRuntime: core.NewServiceRuntime(testCore, AgentOptions{}), + forge: forge.NewForge(serverURL, "test-token"), + forgeURL: serverURL, + forgeToken: "test-token", + backoff: make(map[string]time.Time), + failCount: make(map[string]int), + } +} + +func newCleanupRemoteRepo(t *testing.T) (string, string) { + t.Helper() + + remoteRoot := t.TempDir() + remoteDir := core.JoinPath(remoteRoot, "core", "go-io.git") + require.True(t, fs.EnsureDir(core.PathDir(remoteDir)).OK) + require.True(t, testCore.Process().Run(context.Background(), "git", "init", "--bare", remoteDir).OK) + return remoteRoot, remoteDir +} + +func seedCleanupRemoteBranch(t *testing.T, remoteDir, branch string) { + t.Helper() + + repoDir := core.JoinPath(t.TempDir(), "seed-repo") + createCleanupWorkspaceRepo(t, repoDir, branch) + + require.True(t, testCore.Process().RunIn(context.Background(), repoDir, "git", "remote", "add", "origin", remoteDir).OK) + require.True(t, testCore.Process().RunIn(context.Background(), repoDir, "git", "push", "origin", "dev").OK) + require.True(t, testCore.Process().RunIn(context.Background(), repoDir, "git", "push", "origin", branch).OK) +} + +func createCleanupWorkspaceRepo(t *testing.T, repoDir, branch string) { + t.Helper() + + require.True(t, testCore.Process().Run(context.Background(), "git", "init", "-b", "dev", repoDir).OK) + require.True(t, testCore.Process().RunIn(context.Background(), repoDir, "git", "config", "user.name", "Test").OK) + require.True(t, testCore.Process().RunIn(context.Background(), repoDir, "git", "config", "user.email", "test@test.com").OK) + + require.True(t, fs.Write(core.JoinPath(repoDir, "README.md"), "# cleanup").OK) + require.True(t, testCore.Process().RunIn(context.Background(), repoDir, "git", "add", ".").OK) + require.True(t, testCore.Process().RunIn(context.Background(), repoDir, "git", "commit", "-m", "initial").OK) + + require.True(t, testCore.Process().RunIn(context.Background(), repoDir, "git", "checkout", "-b", branch).OK) + require.True(t, fs.Write(core.JoinPath(repoDir, "README.md"), "# cleanup branch").OK) + require.True(t, testCore.Process().RunIn(context.Background(), repoDir, "git", "add", ".").OK) + require.True(t, testCore.Process().RunIn(context.Background(), repoDir, "git", "commit", "-m", "branch work").OK) +} + +func cleanupRemoteBranchExists(remoteDir, branch string) bool { + ref := core.Concat("refs/heads/", branch) + return testCore.Process().Run(context.Background(), "git", "--git-dir", remoteDir, "show-ref", "--verify", "--quiet", ref).OK +} + +func writeCleanupGitRewrite(t *testing.T, remoteRoot string) { + t.Helper() + + configPath := core.JoinPath(t.TempDir(), "gitconfig") + configBody := core.Sprintf("[url \"%s\"]\n\tinsteadOf = ssh://git@forge.lthn.ai:2223/\n", core.Concat("file://", remoteRoot, "/")) + require.True(t, fs.Write(configPath, configBody).OK) + t.Setenv("GIT_CONFIG_GLOBAL", configPath) +} + +func newCleanupForgeServer(t *testing.T, remoteDir, branch string, deleteStatus int, createPR bool) (*httptest.Server, *cleanupForgeServerState) { + t.Helper() + + state := &cleanupForgeServerState{} + deletePath := core.Concat("/api/v1/repos/core/go-io/branches/", url.PathEscape(branch)) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case createPR && r.Method == http.MethodPost && r.URL.Path == "/api/v1/repos/core/go-io/pulls": + state.prCalls++ + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(core.JSONMarshalString(map[string]any{ + "number": 42, + "html_url": "https://forge.test/core/go-io/pulls/42", + }))) + case r.Method == http.MethodDelete && r.URL.Path == deletePath: + state.deleteCalls++ + if deleteStatus != http.StatusNoContent { + http.Error(w, "delete failed", deleteStatus) + return + } + if remoteDir != "" { + ref := core.Concat("refs/heads/", branch) + deleteResult := testCore.Process().Run(context.Background(), "git", "--git-dir", remoteDir, "update-ref", "-d", ref) + if !deleteResult.OK { + http.Error(w, "delete ref failed", http.StatusInternalServerError) + return + } + } + w.WriteHeader(http.StatusNoContent) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(server.Close) + + return server, state +} diff --git a/pkg/agentic/commands.go b/pkg/agentic/commands.go index 70d3510..e106aea 100644 --- a/pkg/agentic/commands.go +++ b/pkg/agentic/commands.go @@ -468,13 +468,20 @@ func (s *PrepSubsystem) cmdContentSchemaGenerate(options core.Options) core.Resu } func (s *PrepSubsystem) cmdComplete(options core.Options) core.Result { - result := s.handleComplete(s.commandContext(), options) + ctx := s.commandContext() + result := s.handleComplete(ctx, options) if !result.OK { err := commandResultError("agentic.cmdComplete", result) core.Print(nil, "error: %v", err) return core.Result{Value: err, OK: false} } + workspace := optionStringValue(options, "workspace", "_arg") + cleanupResult := s.cleanupWorkspaceBranch(ctx, workspace) + if !cleanupResult.OK { + core.Warn("agentic.cmdComplete: branch cleanup failed", "workspace", workspace, "reason", cleanupResult.Value) + } + return result } diff --git a/pkg/agentic/pr.go b/pkg/agentic/pr.go index 5b217d2..ee97a84 100644 --- a/pkg/agentic/pr.go +++ b/pkg/agentic/pr.go @@ -148,7 +148,10 @@ func (s *PrepSubsystem) createPR(ctx context.Context, _ *mcp.CallToolRequest, in workspaceStatus.PRURL = pullRequestURL writeStatusResult(workspaceDir, workspaceStatus) - s.cleanupForgeBranch(ctx, repoDir, forgeRemote, workspaceStatus.Branch) + cleanupResult := s.cleanupBranch(ctx, cleanupRepoRef(workspaceStatus.Org, workspaceStatus.Repo), workspaceStatus.Branch) + if !cleanupResult.OK { + core.Warn("createPR: branch cleanup failed", "repo", workspaceStatus.Repo, "branch", workspaceStatus.Branch, "reason", cleanupResult.Value) + } if workspaceStatus.Issue > 0 { comment := core.Sprintf("Pull request created: %s", pullRequestURL) @@ -498,12 +501,6 @@ type DeleteBranchOutput struct { // s.deleteBranch(ctx, nil, agentic.DeleteBranchInput{Repo: "go-io", Branch: "agent/fix-tests"}) func (s *PrepSubsystem) deleteBranch(ctx context.Context, _ *mcp.CallToolRequest, input DeleteBranchInput) (*mcp.CallToolResult, DeleteBranchOutput, error) { - if s.forgeToken == "" { - return nil, DeleteBranchOutput{}, core.E("deleteBranch", "no Forge token configured", nil) - } - if s.forge == nil { - return nil, DeleteBranchOutput{}, core.E("deleteBranch", "forge client is not configured", nil) - } if input.Repo == "" || input.Branch == "" { return nil, DeleteBranchOutput{}, core.E("deleteBranch", "repo and branch are required", nil) } @@ -513,15 +510,20 @@ func (s *PrepSubsystem) deleteBranch(ctx context.Context, _ *mcp.CallToolRequest org = "core" } - if err := s.forge.Branches.DeleteBranch(ctx, org, input.Repo, input.Branch); err != nil { - return nil, DeleteBranchOutput{}, core.E("deleteBranch", core.Concat("failed to delete branch ", input.Branch), err) + cleanupResult := s.cleanupBranch(ctx, cleanupRepoRef(org, input.Repo), input.Branch) + if !cleanupResult.OK { + err, _ := cleanupResult.Value.(error) + if err == nil { + err = core.E("deleteBranch", "branch cleanup failed", nil) + } + return nil, DeleteBranchOutput{}, err } return nil, DeleteBranchOutput{ Success: true, Org: org, Repo: input.Repo, - Branch: input.Branch, + Branch: cleanupBranchName(input.Branch), }, nil } diff --git a/tests/cli/branch/Taskfile.yaml b/tests/cli/branch/Taskfile.yaml index b42a192..2dfe414 100644 --- a/tests/cli/branch/Taskfile.yaml +++ b/tests/cli/branch/Taskfile.yaml @@ -4,3 +4,9 @@ tasks: test: cmds: - task -d delete test + - | + bash <<'EOF' + set -euo pipefail + cd ../../.. + go test ./pkg/agentic -run 'TestCleanupBranch_' -count=1 + EOF