Repository navigation
Conversation
pushed_branch() only extracted the refspec SOURCE and used that single name for both the trust-root diff and the default-branch check. That meant: - 'git push origin feature/x:master' checked 'feature/x' against the default branch, never matched, and bypassed default-branch push protection entirely -- despite actually writing to master. - 'git push origin master:feature/x' checked 'master' against the default branch, matched, and wrongly denied a push that never actually touches the default branch. - '--all'/'--mirror' only ever inspected one branch (whichever positional read happened to find), so trust-root changes on other pushed refs could evade classification. Replace pushed_branch() with pushed_refs(), which resolves every pushed ref as a (source, destination) pair: destinations are checked against the default branch (targets_default_branch), sources are diffed for trust-root touches (combined_trust_root_touch, unioned across every ref). --all/--mirror now enumerate every local branch instead of reading a single positional arg. An unresolvable push mode or refspec fails closed (Unknown + targets_default_branch=true) rather than passing through. Add end-to-end regressions for feature:master, master:feature, and a multi-branch --all push.
|
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:
📝 WalkthroughWalkthroughPush classification now resolves all effective source and destination refs, including omitted refspecs, ChangesMulti-ref push classification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GitPush
participant PushRef
participant classify_with_home
GitPush->>PushRef: Provide flags and refspecs
PushRef->>classify_with_home: Return source and destination refs
classify_with_home->>classify_with_home: Check trust-root sources and protected destinations
classify_with_home-->>GitPush: Return classification
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/flare-git-core/src/classify.rs (2)
520-543: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAggregation logic is correct; consider skipping the diffs that cannot change the outcome.
The precedence order (
Touched>Unknown>Clean) and the path union are right, and the argument order matchesresolve_trust_root_touch(repo_root, branch, target, ...).
classify_pureusestrust_root_touchfor a push only whenpush_targets_default_branchistrue. For an--allpush over many local branches, this function still runs onegit diffsubprocess per ref. Computingtargets_default_branchfirst, then callingcombined_trust_root_touchonly when it istrue, removes that work without changing any disposition.🤖 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 `@crates/flare-git-core/src/classify.rs` around lines 520 - 543, Update the push classification flow in classify_pure to compute targets_default_branch first and invoke combined_trust_root_touch only when it is true; otherwise skip the per-ref trust-root diff work while preserving the existing disposition and precedence behavior.
417-419: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
strip_prefixinstead oftrim_start_matches.
trim_start_matchesremoves every repeated occurrence of the pattern.refs/heads/refs/heads/xcollapses tox, which is not a name git would accept. A single strip matches the documented intent.♻️ Proposed single-prefix strip
fn strip_ref_prefix(name: &str) -> String { - name.trim_start_matches("refs/heads/").to_string() + name.strip_prefix("refs/heads/").unwrap_or(name).to_string() }🤖 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 `@crates/flare-git-core/src/classify.rs` around lines 417 - 419, Update strip_ref_prefix to remove at most one leading "refs/heads/" prefix by using the single-prefix operation, preserving the original name when the prefix is absent or repeated rather than collapsing repeated prefixes.
🤖 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 `@crates/flare-git-core/src/classify.rs`:
- Around line 506-513: Update the argument classification logic around the
non_flags handling to remove values belonging to known value-taking options (-o,
--push-option, --receive-pack, --repo, and --exec) before identifying the remote
and refspecs. Also detect tag-only pushes such as origin --tags and return no
branch refs instead of falling back to current_branch; preserve the existing
current-branch fallback only when no refspec or tag-only mode is present.
- Around line 495-505: Update the argument handling before the local_branches
expansion so --mirror is treated as unresolvable and follows the existing
fail-closed path, while --all retains the current local-branch PushRef
expansion. Revise the related doc comments near the --all/--mirror handling to
describe this distinction accurately.
- Around line 446-467: Update parse_refspec to remove a leading force marker
(`+`) from the entire spec before calling split_once(':') or fallback parsing.
Use the normalized refspec for both source and destination extraction so inputs
such as +master and +feature/x produce the same PushRef values as their unmarked
forms.
---
Nitpick comments:
In `@crates/flare-git-core/src/classify.rs`:
- Around line 520-543: Update the push classification flow in classify_pure to
compute targets_default_branch first and invoke combined_trust_root_touch only
when it is true; otherwise skip the per-ref trust-root diff work while
preserving the existing disposition and precedence behavior.
- Around line 417-419: Update strip_ref_prefix to remove at most one leading
"refs/heads/" prefix by using the single-prefix operation, preserving the
original name when the prefix is absent or repeated rather than collapsing
repeated prefixes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c7b21fd-c526-4dbd-aecf-d5dacabeee21
📒 Files selected for processing (1)
crates/flare-git-core/src/classify.rs
- parse_refspec: strip a leading '+' force marker before parsing -- '+master' previously failed to match the default branch name and let a force push of the default branch slip the direct-push guard. - pushed_refs: treat --mirror as unresolvable (fail closed) instead of approximating it as --all's local-branch set, since --mirror's write surface (branches, tags, remote-only deletions) is strictly larger than local_branches() enumerates. - pushed_refs: skip the value token following -o/--push-option/ --receive-pack/--repo/--exec so it can't be misread as the remote or a refspec; treat a --tags push with no explicit refspec as pushing no branches instead of falling back to the current branch. - strip_ref_prefix: use strip_prefix instead of trim_start_matches so a repeated refs/heads/ prefix isn't collapsed past a single strip. - classify_with_home: resolve targets_default_branch first and only run the trust-root diff subprocesses when it's true, since every classify_pure match arm already gates on that flag. 5 new regression tests.
5 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pushed_branch()incrates/flare-git-core/src/classify.rsonly extracted a refspec's SOURCE and used that single name for both the trust-root diff and the "does this target the default branch" check.git push origin feature/x:mastercheckedfeature/xagainst the default branch, never matched, and bypassed default-branch push protection entirely — despite actually writing tomaster.git push origin master:feature/xcheckedmasteragainst the default branch, matched, and wrongly denied a push that never actually touches the default branch.--all/--mirroronly ever inspected one branch (whatever a positional read happened to find), so trust-root changes on other pushed refs could evade classification.Fix
Replaced
pushed_branch()withpushed_refs(), resolving every pushed ref as a(source, destination)pair:targets_default_branch)combined_trust_root_touch, unioned across every ref)--all/--mirrornow enumerate every local branch instead of reading a single positional argUnknown+targets_default_branch=true) instead of passing throughTesting
cargo test -p flare-git-core— 141 passed, 0 failed, including new e2e regressions forfeature:master(now denied),master:feature(now passes through, previously a false-positive deny), and a multi-branch--allpushcargo fmt --checkcleancargo clippy --workspace --all-targets --all-features -- -A unsafe_code -A clippy::pedantic -D warningscleanSummary by CodeRabbit
--allor--mirroroperations.