Skip to content

prevent creating cyclical dependencies#649

Merged
aviator-app[bot] merged 2 commits intomasterfrom
fix_cyclical_deps
Jan 21, 2026
Merged

prevent creating cyclical dependencies#649
aviator-app[bot] merged 2 commits intomasterfrom
fix_cyclical_deps

Conversation

@tulioz
Copy link
Copy Markdown
Contributor

@tulioz tulioz commented Jan 19, 2026

No description provided.

@tulioz tulioz requested a review from a team as a code owner January 19, 2026 21:13
@aviator-app
Copy link
Copy Markdown
Contributor

aviator-app bot commented Jan 19, 2026

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged using Aviator.


See the real-time status of this PR on the Aviator webapp.
Use the Aviator Chrome Extension to see the status of your PR within GitHub.

@aviator-app
Copy link
Copy Markdown
Contributor

aviator-app bot commented Jan 19, 2026

✅ FlexReview Status

Common Owner: aviator-co/engineering (expert-load-balance assignment)
Owner and Assignment:

  • aviator-co/engineering (expert-load-balance assignment)
    Owned Files
    • 🔒 cmd/av/adopt.go
    • 🔒 cmd/av/branchmeta.go
    • 🔒 cmd/av/main.go
    • 🔒 cmd/av/validate_db.go
    • 🔒 internal/actions/adopt_branches.go
    • 🔒 internal/meta/branch.go
    • 🔒 internal/meta/branch_test.go
    • 🔒 internal/reorder/stackbranch.go
    • 🔒 internal/reorder/stackbranch_test.go

Review SLO: 7 business hours if PR size is <= 200 LOC for the first response.
❕ This PR modifies 319 lines, which is larger than the Review SLO threshold.

@aviator-app aviator-app bot requested a review from davi-maciel January 19, 2026 21:13
@gemini-code-assist
Copy link
Copy Markdown

Summary of Changes

Hello @tulioz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the robustness of the av CLI by implementing comprehensive validation to prevent cyclical dependencies within branch stack metadata. By integrating this validation into critical branch management commands and providing a dedicated validate-db tool, it ensures that the branch relationships remain consistent and manageable, thereby improving the overall reliability and user experience of the av system.

Highlights

  • Cyclical Dependency Prevention: Introduced a new ValidateNoCycle function to detect and prevent cyclical dependencies in branch stacks, ensuring the integrity of the branch metadata.
  • Integrated Validation: The ValidateNoCycle check has been integrated into key commands: av adopt, av branch meta set, and av reorder stack-branch. These commands will now error if an action would create a cyclical dependency.
  • New av validate-db Command: Added a new command, av validate-db, which allows users to audit their av metadata for consistency issues. This includes checking for cyclical dependencies, missing parent branches, self-referencing parents, and branches that exist in metadata but not in the Git repository.
  • Enhanced Testing: New unit tests have been added for the ValidateNoCycle function and for the reorder stack-branch command to specifically cover cases where cyclical dependencies are rejected.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@tulioz tulioz force-pushed the fix_cyclical_deps branch from 29e685c to e7d3062 Compare January 19, 2026 21:17
@tulioz
Copy link
Copy Markdown
Contributor Author

tulioz commented Jan 19, 2026

/gemini review

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request effectively prevents the creation of cyclical branch dependencies by introducing a ValidateNoCycle function and integrating it into commands that modify branch parentage, such as adopt, branch-meta set, and reorder. A new av validate-db command is also added to help diagnose existing metadata inconsistencies, which is a great addition for repository health. The implementation is robust and includes relevant tests. I have one suggestion to remove a redundant check in the new validation command to avoid duplicate error reporting.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a mechanism to prevent cyclical dependencies between branches, which is a crucial safeguard for maintaining a healthy branch stack structure. The core logic is implemented in the new ValidateNoCycle function, which is correctly integrated into various commands that modify branch parent-child relationships, such as adopt, branch-meta set, and reorder.

The addition of the av validate-db command is a great enhancement, providing a tool to diagnose inconsistencies in the av metadata, including the newly-covered cyclical dependencies. The test coverage for the new logic is also solid.

I have one suggestion to improve the diagnostic output of the validate-db command to avoid reporting duplicate errors for the same underlying issue.

Comment on lines +71 to +95
if branch.Parent.Name == branchName {
issues = append(issues, diagnosticIssue{
severity: diagnosticError,
branch: branchName,
message: "parent points to itself",
})
}

if !branch.Parent.Trunk && branch.Parent.Name != "" {
if _, ok := tx.Branch(branch.Parent.Name); !ok {
issues = append(issues, diagnosticIssue{
severity: diagnosticError,
branch: branchName,
message: fmt.Sprintf("parent %q is missing from av metadata", branch.Parent.Name),
})
}
}

if err := meta.ValidateNoCycle(tx, branchName, branch.Parent); err != nil {
issues = append(issues, diagnosticIssue{
severity: diagnosticError,
branch: branchName,
message: "parent chain introduces cyclical branch dependencies",
})
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The check for a branch being its own parent (a self-loop) on lines 71-77 is a specific case of the general cyclical dependency check on lines 89-95. As meta.ValidateNoCycle also detects self-loops, running both checks unconditionally will result in duplicate error messages for the same issue.

To provide clearer diagnostics, it's better to combine these checks. You can use an else if to run the general cycle detection only if the more specific self-loop check fails. This avoids redundant error reporting and makes the validation output more precise for the user.

		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:  "parent chain introduces cyclical branch dependencies",
			})
		}

		if !branch.Parent.Trunk && branch.Parent.Name != "" {
			if _, ok := tx.Branch(branch.Parent.Name); !ok {
				issues = append(issues, diagnosticIssue{
					severity: diagnosticError,
					branch:   branchName,
					message:  fmt.Sprintf("parent %q is missing from av metadata", branch.Parent.Name),
				})
			}
		}

@davi-maciel davi-maciel removed their request for review January 19, 2026 21:41
visited[current] = true

branch, ok := tx.Branch(current)
if !ok {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we be returning here also?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We return nil here

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we return the error here?

current := parent.Name
for current != "" {
if visited[current] {
return errors.New("would introduce cyclical branch dependencies")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this error message surfaced anywhere?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not directly, but the callers to this method will post specific errors, for example in adopt:

			"could not adopt branch %q because it would introduce cyclical branch dependencies",
			currentBranch,
		)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean this particular error message string is irrelevant then? Consider returning an empty string avoid confusion

})
}

exists, err := repo.DoesBranchExist(ctx, branchName)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this checking for remote branch? If not, shouldn't this check be done first. If the branch doesn't exist, rest of the checks would be invalid?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reordered, I only expect us to need this command in extreme edge cases already (this is the av validate-db command, not part of a normal operation).

@tulioz tulioz force-pushed the fix_cyclical_deps branch from e7d3062 to fb5cc90 Compare January 21, 2026 21:08
@tulioz tulioz force-pushed the fix_cyclical_deps branch from fb5cc90 to 29e0ba0 Compare January 21, 2026 21:14
@tulioz
Copy link
Copy Markdown
Contributor Author

tulioz commented Jan 21, 2026

/gemini review

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces crucial validation to prevent cyclical dependencies in branch stacks by adding a ValidateNoCycle function and integrating it across the codebase where branch parents are modified. It also adds a new av validate-db command to help users diagnose metadata inconsistencies. The changes are well-structured and include relevant tests. I have a couple of suggestions to improve an error message for better clarity and to simplify some of the new validation logic.

@tulioz tulioz force-pushed the fix_cyclical_deps branch from d00307f to 83ed0a2 Compare January 21, 2026 21:40
@aviator-app aviator-app bot merged commit 10d3ae9 into master Jan 21, 2026
6 of 8 checks passed
@aviator-app aviator-app bot deleted the fix_cyclical_deps branch January 21, 2026 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants