feat(agent/pipeline): wire branch cleanup into createPR + cmdComplete success paths (#545)
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 <noreply@openai.com>
Closes tasks.lthn.sh/view.php?id=545
This commit is contained in:
parent
92b433ad76
commit
23f4be5b85
5 changed files with 396 additions and 11 deletions
121
pkg/agentic/branch_cleanup.go
Normal file
121
pkg/agentic/branch_cleanup.go
Normal file
|
|
@ -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 <repo> or <org/repo>", 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
|
||||
}
|
||||
}
|
||||
249
pkg/agentic/branch_cleanup_test.go
Normal file
249
pkg/agentic/branch_cleanup_test.go
Normal file
|
|
@ -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
|
||||
}
|
||||
|
|
@ -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
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue