Repository navigation
Conversation
🔃 FlexReview StatusCommon Owner:
Review SLO: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new --include ancestors flag to the av adopt command, enabling non-interactive adoption of a remote branch and its ancestors. It includes updates to the command-line documentation, enhances the mock GitHub GraphQL server in end-to-end tests to support querying pull requests by number, and adds corresponding test coverage. The feedback suggests registering a shell completion function for the new --include flag to improve the CLI user experience.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| adoptCmd.Flags().StringVar( | ||
| &adoptFlags.Include, "include", "", | ||
| "with --remote, non-interactively adopt the named branch and related branches\n(ancestors)", | ||
| ) |
There was a problem hiding this comment.
To improve the CLI user experience, we can register a shell completion function for the new --include flag. Since the only valid value currently is ancestors (defined in adoptIncludeValues), we can provide this directly to Cobra's shell completion mechanism.
adoptCmd.Flags().StringVar(
&adoptFlags.Include, "include", "",
"with --remote, non-interactively adopt the named branch and related branches\n(ancestors)",
)
_ = adoptCmd.RegisterFlagCompletionFunc(
"include",
func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
return adoptIncludeValues, cobra.ShellCompDirectiveNoFileComp
},
)
av adopt --remote opens an interactive picker, which is awkward for scripts and agents mirroring a coworker's stack. Add --include ancestors to skip the picker and adopt the named branch plus its ancestors up to the trunk in one shot. --dry-run prints the adoption plan. Also extend the e2e harness: the mock GitHub server now answers the pullRequest(number:) query used to walk parent PRs, and mock-pull accepts base=<ref> and body=<file> so a remote stack with av metadata can be modeled in tests.
f364378 to
0efd2fc
Compare
Current Aviator status
This pull request is currently open (not queued). How to mergeTo merge this PR, comment
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.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e_tests/mock_ghgql_server.go (1)
141-158: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnchecked type assertion can panic the mock server.
req.Variables["number"].(float64)panics if the variable is missing or of an unexpected type, crashing the mock HTTP handler goroutine with an unclear stack trace instead of a controlled test failure.🛡️ Proposed fix using comma-ok assertion
func (s *mockGitHubServer) handlePRByNumberQuery(req graphqlRequest) graphqlResponse { // JSON numbers decode to float64. - number := int(req.Variables["number"].(float64)) + numberFloat, ok := req.Variables["number"].(float64) + if !ok { + s.t.Fatalf("handlePRByNumberQuery: missing or invalid 'number' variable: %v", req.Variables["number"]) + } + number := int(numberFloat)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e_tests/mock_ghgql_server.go` around lines 141 - 158, The handlePRByNumberQuery helper in mockGitHubServer should not use an unchecked type assertion on req.Variables["number"]; it can panic if the variable is missing or not numeric. Update the number extraction to use a comma-ok style check, and if the variable is absent or the wrong type, return a controlled graphqlResponse (or fail the mock request explicitly) instead of crashing the handler. Keep the fix scoped to handlePRByNumberQuery and the req.Variables["number"] lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/av/adopt.go`:
- Around line 336-349: The dry-run preview in adopt.go is using
prInfo.Parent.Name directly, so it can report a stale parent instead of the same
open ancestor/trunk chosen by the real adoption flow. Update the dry-run block
in the adoption command to reuse the same parent-resolution logic as
initAdoption/adopt (the reparent walk) when building the “Would adopt” lines, so
the displayed parent matches what actual adoption would use.
---
Nitpick comments:
In `@e2e_tests/mock_ghgql_server.go`:
- Around line 141-158: The handlePRByNumberQuery helper in mockGitHubServer
should not use an unchecked type assertion on req.Variables["number"]; it can
panic if the variable is missing or not numeric. Update the number extraction to
use a comma-ok style check, and if the variable is absent or the wrong type,
return a controlled graphqlResponse (or fail the mock request explicitly)
instead of crashing the handler. Keep the fix scoped to handlePRByNumberQuery
and the req.Variables["number"] lookup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6183aa94-6e30-4d9c-a833-0b28366cd89c
📒 Files selected for processing (5)
cmd/av/adopt.godocs/av-adopt.1.mde2e_tests/mock_ghgql_server.goe2e_tests/testdata/script/adopt_remote_include_ancestors.txtare2e_tests/testscript_test.go
| if adoptFlags.DryRun { | ||
| parents := make(map[string]string, len(prs)) | ||
| for _, prInfo := range prs { | ||
| parents[prInfo.Name] = prInfo.Parent.Name | ||
| } | ||
| lines := []string{"Would adopt:"} | ||
| for _, target := range adoptionTargets { | ||
| lines = append(lines, fmt.Sprintf(" %s (parent: %s)", target.Short(), parents[target.Short()])) | ||
| } | ||
| return tea.Batch( | ||
| vm.AddView(uiutils.SimpleMessageView{Message: colors.SuccessStyle.Render(strings.Join(lines, "\n"))}), | ||
| tea.Quit, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 'PullRequestState(Merged|Closed)|Parent\.Trunk|prMap\[' cmd/av/adopt.goRepository: aviator-co/av
Length of output: 665
🏁 Script executed:
#!/bin/bash
sed -n '330,450p' cmd/av/adopt.goRepository: aviator-co/av
Length of output: 3934
🏁 Script executed:
#!/bin/bash
sed -n '250,340p' cmd/av/adopt.goRepository: aviator-co/av
Length of output: 2620
🏁 Script executed:
#!/bin/bash
rg -n "type RemotePRInfo|NewGetRemoteStackedPRModel|GetRemoteStackedPRModel|type AdoptingBranch|type BranchState" -S .Repository: aviator-co/av
Length of output: 982
🏁 Script executed:
#!/bin/bash
sed -n '1,220p' internal/actions/get_remote_stacked_pr.go && printf '\n---\n' && sed -n '1,120p' internal/actions/adopt_branches.go && printf '\n---\n' && sed -n '1,120p' internal/meta/branchstate.goRepository: aviator-co/av
Length of output: 9163
Dry-run should use the same reparent walk as adoption. prInfo.Parent.Name can still point to a merged/closed PR, so Would adopt may show a parent that differs from the branch actually adopted after initAdoption walks up to an open ancestor or trunk.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/av/adopt.go` around lines 336 - 349, The dry-run preview in adopt.go is
using prInfo.Parent.Name directly, so it can report a stale parent instead of
the same open ancestor/trunk chosen by the real adoption flow. Update the
dry-run block in the adoption command to reuse the same parent-resolution logic
as initAdoption/adopt (the reparent walk) when building the “Would adopt” lines,
so the displayed parent matches what actual adoption would use.
pls 🙏 my Claude needs non-interactive adoption of entire stacks
What
av adopt --remote <branch>always drops into the interactive picker, which is a wall for scripts and agents trying to mirror a coworker's stack locally. This adds an--include ancestorsflag that skips the picker:It adopts the named branch and its ancestor chain up to the trunk without prompting. Bare
--remotestill opens the picker, so this is non-breaking.--includeis a validated string flag (onlyancestorstoday) so it can grow todescendants/stacklater without adding new flags.Tests
Extends the e2e harness, which had no coverage of the remote-adopt path:
pullRequest(number:)query used to walk parent PRsmock-pullacceptsbase=<ref>andbody=<file>so a remote stack with av stack metadata can be modeledNew testscript covers the
--dry-runplan and--includevalidation.Stacked on #755.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
av adopt --remotenow supports--include=ancestorsfor non-interactive adoption of a remote branch and its ancestor branches.Documentation
--includeoption.Bug Fixes