prevent creating cyclical dependencies

cmd/av/adopt.go

@@ -116,7 +116,6 @@ func adoptForceAdoption(
 			Name:  parent,
 			Trunk: true,
 		}
-		tx.SetBranch(branch)
 	} else {
 		_, exist := tx.Branch(parent)
 		if !exist {
@@ -131,8 +130,12 @@ func adoptForceAdoption(
 			Trunk:                    false,
 			BranchingPointCommitHash: mergeBase,
 		}
-		tx.SetBranch(branch)
 	}
+
+	if err := meta.ValidateNoCycle(tx, currentBranch, branch.Parent); err != nil {
+		return err
+	}
+	tx.SetBranch(branch)
 	if adoptFlags.DryRun {
 		return nil
 	}

cmd/av/branchmeta.go

@@ -112,6 +112,9 @@ var branchMetaSetCmd = &cobra.Command{
 				Trunk:                    branchMetaFlags.trunk,
 				BranchingPointCommitHash: parentHead,
 			}
+			if err := meta.ValidateNoCycle(tx, args[0], br.Parent); err != nil {
+				return err
+			}
 		}
 		tx.SetBranch(br)
 		return tx.Commit()

cmd/av/main.go

@@ -100,6 +100,7 @@ func init() {
 		branchCmd,
 		branchMetaCmd,
 		commitCmd,
+		validateDBCmd,
 		diffCmd,
 		fetchCmd,
 		initCmd,

cmd/av/validate_db.go

@@ -0,0 +1,145 @@
+package main
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/aviator-co/av/internal/git"
+	"github.com/aviator-co/av/internal/meta"
+	"github.com/aviator-co/av/internal/utils/colors"
+	"github.com/charmbracelet/lipgloss"
+	"github.com/spf13/cobra"
+)
+
+type diagnosticSeverity string
+
+const (
+	diagnosticError   diagnosticSeverity = "error"
+	diagnosticWarning diagnosticSeverity = "warning"
+)
+
+type diagnosticIssue struct {
+	severity diagnosticSeverity
+	branch   string
+	message  string
+}
+
+var validateDBCmd = &cobra.Command{
+	Use:   "validate-db",
+	Short: "Validate av metadata",
+	Long: `Validate av metadata for common consistency issues, including cyclical
+branch dependencies and missing parents.`,
+	Args: cobra.NoArgs,
+	RunE: func(cmd *cobra.Command, _ []string) error {
+		ctx := cmd.Context()
+		repo, err := getRepo(ctx)
+		if err != nil {
+			return err
+		}
+		db, err := getDB(ctx, repo)
+		if err != nil {
+			return err
+		}
+
+		issues, err := validateDB(ctx, repo, db.ReadTx())
+		if err != nil {
+			return err
+		}
+
+		fmt.Print(renderValidation(issues))
+		return nil
+	},
+}
+
+func validateDB(ctx context.Context, repo *git.Repo, tx meta.ReadTx) ([]diagnosticIssue, error) {
+	branches := tx.AllBranches()
+	issues := make([]diagnosticIssue, 0)
+
+	for branchName, branch := range branches {
+		if branchName == "" {
+			continue
+		}
+
+		exists, err := repo.DoesBranchExist(ctx, branchName)
+		if err != nil {
+			return nil, err
+		}
+		if !exists {
+			issues = append(issues, diagnosticIssue{
+				severity: diagnosticError,
+				branch:   branchName,
+				message:  "branch is missing from the Git repository",
+			})
+			continue
+		}
+
+		if branch.Parent.Name == "" && !branch.Parent.Trunk {
+			issues = append(issues, diagnosticIssue{
+				severity: diagnosticError,
+				branch:   branchName,
+				message:  "parent is empty but not marked as trunk",
+			})
+		}
+
+		if branch.Parent.Name == branchName {
+			issues = append(issues, diagnosticIssue{
+				severity: diagnosticError,
+				branch:   branchName,
+				message:  "parent points to itself",
+			})
+		} else if err := meta.ValidateNoCycle(tx, branchName, branch.Parent); err != nil {
+			issues = append(issues, diagnosticIssue{
+				severity: diagnosticError,
+				branch:   branchName,
+				message:  err.Error(),
+			})
+		}
+	}
+
+	return issues, nil
+}
+
+func renderValidation(issues []diagnosticIssue) string {
+	if len(issues) == 0 {
+		return lipgloss.NewStyle().MarginTop(1).MarginBottom(1).MarginLeft(2).Render(
+			colors.SuccessStyle.Render("✓ No av metadata issues found"),
+		) + "\n"
+	}
+
+	var errors, warnings []diagnosticIssue
+	for _, issue := range issues {
+		switch issue.severity {
+		case diagnosticError:
+			errors = append(errors, issue)
+		case diagnosticWarning:
+			warnings = append(warnings, issue)
+		}
+	}
+
+	var ss []string
+	if len(errors) > 0 {
+		ss = append(ss, colors.FailureStyle.Render("✗ Issues found in av metadata"))
+	} else {
+		ss = append(ss, colors.Warning("! Warnings found in av metadata"))
+	}
+
+	if len(errors) > 0 {
+		ss = append(ss, "")
+		ss = append(ss, "  Errors:")
+		for _, issue := range errors {
+			ss = append(ss, fmt.Sprintf("  * %s: %s", issue.branch, issue.message))
+		}
+	}
+
+	if len(warnings) > 0 {
+		ss = append(ss, "")
+		ss = append(ss, "  Warnings:")
+		for _, issue := range warnings {
+			ss = append(ss, fmt.Sprintf("  * %s: %s", issue.branch, issue.message))
+		}
+	}
+
+	return lipgloss.NewStyle().MarginTop(1).MarginBottom(1).MarginLeft(2).Render(
+		lipgloss.JoinVertical(0, ss...),
+	) + "\n"
+}

internal/actions/adopt_branches.go

@@ -71,6 +71,9 @@ func (m *AdoptBranchesModel) adoptBranches() tea.Msg {
 		bi, _ := tx.Branch(branch.Name)
 		bi.Parent = branch.Parent
 		bi.PullRequest = branch.PullRequest
+		if err := meta.ValidateNoCycle(tx, branch.Name, bi.Parent); err != nil {
+			return err
+		}
 		tx.SetBranch(bi)
 	}
 	if err := tx.Commit(); err != nil {

internal/meta/branch.go

@@ -2,6 +2,7 @@ package meta
 
 import (
 	"encoding/json"
+	"fmt"
 
 	"emperror.dev/errors"
 	"github.com/shurcooL/githubv4"
@@ -170,6 +171,42 @@ func HasExcludedAncestor(tx ReadTx, name string) (bool, string) {
 	return HasExcludedAncestor(tx, branch.Parent.Name)
 }
 
+// ValidateNoCycle returns an error if setting a branch's parent would introduce
+// cyclical dependencies in the stack.
+func ValidateNoCycle(tx ReadTx, branchName string, parent BranchState) error {
+	if parent.Trunk || parent.Name == "" {
+		return nil
+	}
+
+	visited := map[string]bool{branchName: true}
+	current := parent.Name
+	for current != "" {
+		if visited[current] {
+			return fmt.Errorf(
+				"branch %q cannot have parent %q because it would introduce cyclical branch dependencies",
+				branchName,
+				parent.Name,
+			)
+		}
+		visited[current] = true
+
+		branch, ok := tx.Branch(current)
+		if !ok {
+			return fmt.Errorf(
+				"branch %q cannot have parent %q: ancestor branch %q is missing from av metadata",
+				branchName,
+				parent.Name,
+				current,
+			)
+		}
+		if branch.Parent.Trunk {
+			return nil
+		}
+		current = branch.Parent.Name
+	}
+	return nil
+}
+
 // StackBranches returns branches in the stack associated with the given branch.
 func StackBranches(tx ReadTx, name string) ([]string, error) {
 	root, found := Root(tx, name)

internal/meta/branch_test.go

@@ -0,0 +1,84 @@
+package meta
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+type testReadTx struct {
+	branches map[string]Branch
+}
+
+func (tx testReadTx) Repository() Repository {
+	return Repository{}
+}
+
+func (tx testReadTx) Branch(name string) (Branch, bool) {
+	branch, ok := tx.branches[name]
+	if ok && branch.Name == "" {
+		branch.Name = name
+	}
+	return branch, ok
+}
+
+func (tx testReadTx) AllBranches() map[string]Branch {
+	return tx.branches
+}
+
+func TestValidateNoCycle(t *testing.T) {
+	for _, tt := range []struct {
+		name      string
+		tx        ReadTx
+		branch    string
+		parent    BranchState
+		expectErr bool
+	}{
+		{
+			name:   "trunk parent allowed",
+			tx:     testReadTx{branches: map[string]Branch{}},
+			branch: "feature",
+			parent: BranchState{Name: "main", Trunk: true},
+		},
+		{
+			name:      "missing parent rejected",
+			tx:        testReadTx{branches: map[string]Branch{}},
+			branch:    "feature",
+			parent:    BranchState{Name: "missing", Trunk: false},
+			expectErr: true,
+		},
+		{
+			name:      "self parent rejected",
+			tx:        testReadTx{branches: map[string]Branch{}},
+			branch:    "feature",
+			parent:    BranchState{Name: "feature", Trunk: false},
+			expectErr: true,
+		},
+		{
+			name: "cycle in parent chain rejected",
+			tx: testReadTx{branches: map[string]Branch{
+				"b": {Name: "b", Parent: BranchState{Name: "c", Trunk: false}},
+				"c": {Name: "c", Parent: BranchState{Name: "a", Trunk: false}},
+				"a": {Name: "a", Parent: BranchState{Name: "main", Trunk: true}},
+			}},
+			branch:    "a",
+			parent:    BranchState{Name: "b", Trunk: false},
+			expectErr: true,
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			err := ValidateNoCycle(tt.tx, tt.branch, tt.parent)
+			if tt.expectErr {
+				assert.Error(t, err)
+				if tt.name == "missing parent rejected" {
+					assert.Contains(t, err.Error(), "missing from av metadata")
+				}
+				if tt.name == "cycle in parent chain rejected" || tt.name == "self parent rejected" {
+					assert.Contains(t, err.Error(), "cyclical branch dependencies")
+				}
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}

internal/reorder/stackbranch.go

@@ -67,6 +67,9 @@ func (b StackBranchCmd) Execute(ctx *Context) error {
 		parentState.BranchingPointCommitHash = headCommit
 	}
 	branch.Parent = parentState
+	if err := meta.ValidateNoCycle(tx, b.Name, branch.Parent); err != nil {
+		return err
+	}
 	tx.SetBranch(branch)
 
 	if headCommit == "" {

internal/reorder/stackbranch_test.go

@@ -1,9 +1,13 @@
 package reorder
 
 import (
+	"bytes"
 	"testing"
 
+	"github.com/aviator-co/av/internal/git/gittest"
+	"github.com/aviator-co/av/internal/meta"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestStackBranchCmd_String(t *testing.T) {
@@ -20,3 +24,35 @@ func TestStackBranchCmd_String(t *testing.T) {
 		})
 	}
 }
+
+func TestStackBranchCmd_ExecuteRejectsCycle(t *testing.T) {
+	repo := gittest.NewTempRepo(t)
+	db := repo.OpenDB(t)
+	out := &bytes.Buffer{}
+	ctx := &Context{repo.AsAvGitRepo(), db, &State{Branch: "main"}, out}
+
+	repo.Git(t, "switch", "-c", "child")
+	repo.CommitFile(t, "child.txt", "child\n")
+	repo.Git(t, "switch", "main")
+
+	tx := db.WriteTx()
+	tx.SetBranch(meta.Branch{
+		Name: "parent",
+		Parent: meta.BranchState{
+			Name:  "main",
+			Trunk: true,
+		},
+	})
+	tx.SetBranch(meta.Branch{
+		Name: "child",
+		Parent: meta.BranchState{
+			Name:  "parent",
+			Trunk: false,
+		},
+	})
+	require.NoError(t, tx.Commit())
+
+	err := StackBranchCmd{Name: "parent", Parent: "child"}.Execute(ctx)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "cyclical branch dependencies")
+}