Conversation
|
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:
📝 WalkthroughWalkthroughAdds Git default-branch staleness detection, target-repository resolution for mutating tools, and a persisted per-session freshness guard that emits an advisory on the first applicable tool call. ChangesRepository freshness guard
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MutatingTool
participant pre_tool_use
participant TargetRepo
participant GitCore
MutatingTool->>pre_tool_use: First mutating call in session
pre_tool_use->>TargetRepo: Resolve target repository
TargetRepo-->>pre_tool_use: Repository or outside
pre_tool_use->>GitCore: Check commits behind origin default
GitCore-->>pre_tool_use: Branch and commit count
pre_tool_use-->>MutatingTool: Add staleness advisory when needed
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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/hook.rs (1)
301-306: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
resolve_mutating_target_reporuns twice per mutating tool call.redirect_decision(src/hook_redirect.rs) and the new freshness guard (src/hook.rs) each independently resolve the target repo for the same tool invocation, each spawning agit rev-parse --show-toplevelsubprocess. Given the explicit tight PreToolUse time budget called out in comments in this same PR, this is avoidable duplicate subprocess overhead on the hot path.
src/hook.rs#L301-L306: thread theTargetRepoalready resolved byredirect_decisioninto the freshness guard instead of re-resolving it here.src/hook_redirect.rs#L284-L290: expose/return the resolvedTargetRepofromredirect_decision(or resolve it once inpre_tool_useand pass it into bothredirect_decisionand the freshness guard) so both consumers share a single resolution per tool call.🤖 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/hook.rs` around lines 301 - 306, Avoid resolving the mutating target repository twice per tool call: in src/hook_redirect.rs:284-290, expose or return the TargetRepo resolved by redirect_decision (or resolve it once in pre_tool_use) and reuse it for both consumers; in src/hook.rs:301-306, pass that existing TargetRepo into the freshness guard instead of calling resolve_mutating_target_repo again.crates/flare-git-core/src/branch.rs (1)
332-336: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't actually exercise the "no remote" path.
init_repo_with_branch("feature/x")has no origin and nomain/masterbranch, soresolve_default_branch's final fallback (symbolic-ref --short HEAD) resolves to"feature/x"itself — the same ascurrent_branch. That meanscommits_behind_origin_defaultreturnsNonevia the "already on default branch" early-return (line 60), never reaching the fetch/!fetchedcode this test is meant to cover. The test passes, but not for the reason its name implies.Consider initializing the repo on
"master"(matching the assumed-default fallback name) with no remote configured, so the function actually falls through to the fetch attempt and fails there.🤖 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/branch.rs` around lines 332 - 336, The test commits_behind_origin_default_none_without_a_remote currently exits through the same-branch early return instead of exercising the no-remote fetch failure. Initialize the repository on “master” while keeping the repository remote-free, so resolve_default_branch selects the assumed default and commits_behind_origin_default reaches the fetch/!fetched path and returns None.
🤖 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 `@src/hook.rs`:
- Around line 301-313: Reorder the conditions in the staleness-check block so
resolve_mutating_target_repo returns TargetRepo::Found before inserting the
session ID into runtime.staleness_checked_sessions. Ensure non-repo targets do
not consume the per-session gate, while preserving the existing single-check
behavior for resolved repositories.
---
Nitpick comments:
In `@crates/flare-git-core/src/branch.rs`:
- Around line 332-336: The test
commits_behind_origin_default_none_without_a_remote currently exits through the
same-branch early return instead of exercising the no-remote fetch failure.
Initialize the repository on “master” while keeping the repository remote-free,
so resolve_default_branch selects the assumed default and
commits_behind_origin_default reaches the fetch/!fetched path and returns None.
In `@src/hook.rs`:
- Around line 301-306: Avoid resolving the mutating target repository twice per
tool call: in src/hook_redirect.rs:284-290, expose or return the TargetRepo
resolved by redirect_decision (or resolve it once in pre_tool_use) and reuse it
for both consumers; in src/hook.rs:301-306, pass that existing TargetRepo into
the freshness guard instead of calling resolve_mutating_target_repo again.
🪄 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: 912cd741-5ed5-40f4-b429-11de6f90467e
📒 Files selected for processing (5)
crates/flare-git-core/src/branch.rscrates/flare-git-core/src/worktree.rssrc/hook.rssrc/hook_redirect.rssrc/optimize/runtime.rs
| if crate::hook_redirect::MUTATING_TOOLS.contains(&parsed.tool_name.as_str()) | ||
| && runtime | ||
| .staleness_checked_sessions | ||
| .insert(parsed.session_id.clone()) | ||
| && let crate::hook_redirect::TargetRepo::Found(repo) = | ||
| crate::hook_redirect::resolve_mutating_target_repo(parsed.tool_input.as_ref()) | ||
| && let Some((default_branch, commits_behind)) = | ||
| flare_git_core::branch::commits_behind_origin_default(&repo, 1) | ||
| && let Some(reason) = | ||
| flare_git_core::branch::staleness_reason(&default_branch, commits_behind) | ||
| { | ||
| nudges.push(reason); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Per-session gate can be consumed by a non-repo target, permanently skipping the freshness check for the rest of the session.
runtime.staleness_checked_sessions.insert(...) fires as soon as the tool is mutating — before resolve_mutating_target_repo is even evaluated. If the session's first mutating call targets a path outside any git repo (TargetRepo::Outside), the one-shot gate is consumed with no staleness check ever having run, and it will never run again for that session even if a later mutating call targets a genuinely stale repo.
Reorder so the repo must resolve to Found before the session's single check is spent:
🐛 Proposed fix
if crate::hook_redirect::MUTATING_TOOLS.contains(&parsed.tool_name.as_str())
- && runtime
- .staleness_checked_sessions
- .insert(parsed.session_id.clone())
- && let crate::hook_redirect::TargetRepo::Found(repo) =
+ && let crate::hook_redirect::TargetRepo::Found(repo) =
crate::hook_redirect::resolve_mutating_target_repo(parsed.tool_input.as_ref())
+ && runtime
+ .staleness_checked_sessions
+ .insert(parsed.session_id.clone())
&& let Some((default_branch, commits_behind)) =
flare_git_core::branch::commits_behind_origin_default(&repo, 1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if crate::hook_redirect::MUTATING_TOOLS.contains(&parsed.tool_name.as_str()) | |
| && runtime | |
| .staleness_checked_sessions | |
| .insert(parsed.session_id.clone()) | |
| && let crate::hook_redirect::TargetRepo::Found(repo) = | |
| crate::hook_redirect::resolve_mutating_target_repo(parsed.tool_input.as_ref()) | |
| && let Some((default_branch, commits_behind)) = | |
| flare_git_core::branch::commits_behind_origin_default(&repo, 1) | |
| && let Some(reason) = | |
| flare_git_core::branch::staleness_reason(&default_branch, commits_behind) | |
| { | |
| nudges.push(reason); | |
| } | |
| if crate::hook_redirect::MUTATING_TOOLS.contains(&parsed.tool_name.as_str()) | |
| && let crate::hook_redirect::TargetRepo::Found(repo) = | |
| crate::hook_redirect::resolve_mutating_target_repo(parsed.tool_input.as_ref()) | |
| && runtime | |
| .staleness_checked_sessions | |
| .insert(parsed.session_id.clone()) | |
| && let Some((default_branch, commits_behind)) = | |
| flare_git_core::branch::commits_behind_origin_default(&repo, 1) | |
| && let Some(reason) = | |
| flare_git_core::branch::staleness_reason(&default_branch, commits_behind) | |
| { | |
| nudges.push(reason); | |
| } |
🤖 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/hook.rs` around lines 301 - 313, Reorder the conditions in the
staleness-check block so resolve_mutating_target_repo returns TargetRepo::Found
before inserting the session ID into runtime.staleness_checked_sessions. Ensure
non-repo targets do not consume the per-session gate, while preserving the
existing single-check behavior for resolved repositories.
getappz
changed the title
1 participant
Auto-opened on
item donefor 019f7582-475d-7bb2-89dc-7d9a778c4522.Summary by CodeRabbit
filePathinputs.