Conversation
…ault branch via API on repo override
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a GitHub repository helper to retrieve default branches, exports it, validates ChangesFlare GitHub workflow handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant flare_git_impl
participant repos_get_default_branch
participant GitHub_Client
flare_git_impl->>flare_git_impl: validate req.action
flare_git_impl->>repos_get_default_branch: resolve repository default branch
repos_get_default_branch->>GitHub_Client: GET repository
GitHub_Client-->>repos_get_default_branch: default_branch
repos_get_default_branch-->>flare_git_impl: branch name
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (2)
src/mcp_server/flare_git.rs (2)
289-296: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for default-branch inference.
The new explicit-
repopath is not exercised by the added tests. Verify that an omittedgit_refrequests the repository’sdefault_branch, and that the no-override path still uses local branch resolution.🤖 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 `@src/mcp_server/flare_git.rs` around lines 289 - 296, Extend the regression tests for the request handling around the `None => match &req.repo` logic to cover both default-branch paths: when `repo` is explicitly provided and `git_ref` is omitted, assert that the repository’s `default_branch` is requested; when no repository override is provided, assert that local `resolve_default_branch` behavior remains in use.
9-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAvoid maintaining the action list in multiple places.
The allowlist duplicates the action names encoded by the dispatch
matchand request schema. A future handler added to only one location will either be rejected early or fall through later. Parse the action once into a shared enum/validated type, or centralize the supported-action definition.🤖 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 `@src/mcp_server/flare_git.rs` around lines 9 - 35, Remove the duplicated KNOWN_ACTIONS allowlist and centralize action validation with the existing dispatch match and request schema. Parse req.action once into a shared enum or validated action type, then use that result for dispatch so newly supported handlers cannot diverge between validation and execution.
🤖 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.
Nitpick comments:
In `@src/mcp_server/flare_git.rs`:
- Around line 289-296: Extend the regression tests for the request handling
around the `None => match &req.repo` logic to cover both default-branch paths:
when `repo` is explicitly provided and `git_ref` is omitted, assert that the
repository’s `default_branch` is requested; when no repository override is
provided, assert that local `resolve_default_branch` behavior remains in use.
- Around line 9-35: Remove the duplicated KNOWN_ACTIONS allowlist and centralize
action validation with the existing dispatch match and request schema. Parse
req.action once into a shared enum or validated action type, then use that
result for dispatch so newly supported handlers cannot diverge between
validation and execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3900c0ea-8a36-4993-b101-88c405651c37
📒 Files selected for processing (3)
src/github/mod.rssrc/github/repos.rssrc/mcp_server/flare_git.rs
|
Addressed the default-branch regression-coverage nitpick: extracted Skipping the KNOWN_ACTIONS-dedup suggestion (centralize into a shared enum/validated type) — real long-term improvement, but a heavier refactor than this bug fix warrants; left a sync-reminder comment on the const instead. Can file a follow-up if desired. |
1 participant
Summary
actionagainst a known list before resolving the repo/creating the GitHub client, so an unknown action fails on its own merits instead of surfacing as a misleading "no repo"/auth error (flare_git.rs:22).github::repos::get_default_branch(GET /repos/{owner}/{repo}) and use it inworkflow_dispatchwhengit_refis omitted with an explicitrepooverride, resolving that repo's actual default branch via the GitHub API instead of requiringgit_ref(flare_git.rs:242).Fixes item #181 (CodeRabbit findings from PR #253's review; pre-existing code moved verbatim in that split, not introduced by it).
Test plan
cargo build --workspace --all-featurescargo clippy --workspace --all-targets --all-features -- -D warnings -A unsafe_code -A clippy::pedanticcargo test --workspace --verbose(51 suites, all green)cargo fmt --checkSummary by CodeRabbit
New Features
Bug Fixes
Tests