Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughAdds Windows-aware local executable resolution and path validation, ownership-scoped dirty-worktree cleanup, and resolved Git/GitHub CLI execution. Local preparation, authentication, tests, state handling, and a TUI scenario are updated. ChangesWindows local workflows
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
OpenCodeReview — PR #288
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/git_info/mod.rs (1)
203-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared
git_command()helper; still silently swallows tool-resolution errors.
crate::local_command::command(LocalTool::Git).ok()?is duplicated verbatim inprobe_branch,probe_short_commit, anddetect_origin_shortform. As previously noted for this pattern, aLocalToolError(e.g. an invalidJEFE_GIT_BINoverride) is silently converted toNone, indistinguishable from "not a git repo." Consider a smallgit_command() -> Option<Command>(orResult) helper mirroringgithub::mod::gh_command()'s approach, centralizing both the resolution call and any future error surfacing.♻️ Proposed helper
+fn git_command() -> Option<Command> { + crate::local_command::command(crate::local_command::LocalTool::Git).ok() +} + fn probe_branch(work_dir: &Path) -> Option<String> { - let output = crate::local_command::command(crate::local_command::LocalTool::Git) - .ok()? + let output = git_command()? .arg("-C")🤖 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/git_info/mod.rs` around lines 203 - 260, Extract the duplicated Git command construction into a shared git_command helper, modeled on github::mod::gh_command(), and use it from probe_branch, probe_short_commit, and detect_origin_shortform. Centralize LocalTool::Git resolution so tool-resolution failures are surfaced consistently rather than each caller silently applying ok()?; preserve the existing command arguments and None behavior for command execution or Git failures.
🤖 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/app_input/issue_cleanup.rs`:
- Around line 33-60: Update restore_tracked_paths to batch the paths returned by
parse_changed_paths into git restore invocations instead of spawning one process
per file. Build each command with the existing --source=HEAD, --staged, and
--worktree arguments, split batches to respect OS argument-length limits, and
preserve git_require_success error handling for every invocation.
- Around line 12-31: Update discard_workdir_changes to restore tracked paths
before deleting untracked entries, and add rollback handling so a failure during
restore does not leave the worktree partially modified. Ensure cleanup also
discovers and removes genuinely empty untracked directories, not just paths
returned by parse_changed_paths from git ls-files.
- Around line 127-149: Update remove_empty_untracked_parents to treat a
remove_dir error caused by an already-absent directory as successful cleanup,
advancing or terminating the parent traversal without returning an error.
Preserve the existing behavior for non-empty directories and other genuine
removal failures, and add coverage for multiple untracked paths sharing the same
parent.
- Around line 106-125: Update remove_untracked_path to identify Windows
directory symlinks or junctions as directory-like reparse points and route them
through remove_dir instead of remove_file, while preserving regular-file and
real-directory behavior. Add a test covering cleanup of an untracked directory
symlink or junction and confirming removal succeeds.
- Around line 62-73: The owned-directory check in is_owned_path currently
compares metadata names case-sensitively; update its component comparisons to
use ASCII case-insensitive matching for both ".jefe" and ".llxprt", while
preserving the existing first-component filtering behavior.
In `@src/runtime/gh_auth.rs`:
- Around line 58-61: Update run_device_auth’s LocalTool::Gh resolution error
mapping to distinguish LocalToolError::InvalidOverride and return
GhError::ToolResolution, matching the behavior in github::mod::gh_command();
keep genuine missing-tool errors mapped to GhError::NotInstalled.
In `@src/services/normalize.rs`:
- Around line 49-72: Update normalize_local_path to detect Windows drive-letter
prefixes and preserve the drive segment as an anchor during .. collapsing.
Ensure paths such as C:\..\foo normalize to a path retaining the drive prefix
rather than popping it or becoming relative, while preserving existing Unix and
non-drive Windows behavior.
---
Outside diff comments:
In `@src/git_info/mod.rs`:
- Around line 203-260: Extract the duplicated Git command construction into a
shared git_command helper, modeled on github::mod::gh_command(), and use it from
probe_branch, probe_short_commit, and detect_origin_shortform. Centralize
LocalTool::Git resolution so tool-resolution failures are surfaced consistently
rather than each caller silently applying ok()?; preserve the existing command
arguments and None behavior for command execution or Git failures.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e240e4ee-9009-4412-b2b8-bd44dcde8f0b
⛔ Files ignored due to path filters (1)
project-plans/issue261-plan.mdis excluded by!project-plans/**
📒 Files selected for processing (20)
dev-docs/tmux-scenarios/windows-local-repository-prep.jsonsrc/app_input/issue_cleanup.rssrc/app_input/issue_git_prep.rssrc/app_input/issue_prep.rssrc/app_input/issue_prep_tests.rssrc/app_shell.rssrc/git_info/mod.rssrc/github/actions.rssrc/github/error.rssrc/github/mod.rssrc/lib.rssrc/local_command.rssrc/runtime/gh_auth.rssrc/services/mod.rssrc/services/normalize.rssrc/state/form_build.rssrc/state/issues_property_ops.rssrc/state/issues_tests_send_to_agent.rssrc/state/prs_property_ops.rssrc/ui/orchestration.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Native Windows (MSVC + psmux)
- GitHub Check: OpenCodeReview
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-04-04T21:05:37.792Z
Learnt from: acoliver
Repo: vybestack/llxprt-jefe PR: 36
File: src/github/mod.rs:537-542
Timestamp: 2026-04-04T21:05:37.792Z
Learning: In Rust, it’s acceptable for function parameters to be intentionally unused when their names start with an underscore (e.g., `_cursor: Option<&str>`). If the parameter name is underscore-prefixed, treat it as a deliberate forward-compatibility/API-consistency choice and do not raise an “unused parameter”/dead code warning during code review.
Applied to files:
src/state/issues_tests_send_to_agent.rssrc/ui/orchestration.rssrc/github/error.rssrc/app_shell.rssrc/state/prs_property_ops.rssrc/app_input/issue_prep.rssrc/lib.rssrc/state/form_build.rssrc/services/mod.rssrc/state/issues_property_ops.rssrc/runtime/gh_auth.rssrc/git_info/mod.rssrc/services/normalize.rssrc/local_command.rssrc/app_input/issue_prep_tests.rssrc/github/actions.rssrc/github/mod.rssrc/app_input/issue_cleanup.rssrc/app_input/issue_git_prep.rs
📚 Learning: 2026-06-22T22:25:52.693Z
Learnt from: acoliver
Repo: vybestack/llxprt-jefe PR: 81
File: src/app_init.rs:11-13
Timestamp: 2026-06-22T22:25:52.693Z
Learning: In Rust, when you call a method that is defined on a trait (e.g., `spawn_session` from `jefe::runtime::RuntimeManager`) on a concrete type (e.g., `TmuxRuntimeManager`), ensure the defining trait is imported into scope (`use ...::RuntimeManager`) if it is not already in scope. Otherwise the call may fail with a “method not found” error because Rust’s method-call resolution requires the trait to be in scope.
Applied to files:
src/state/issues_tests_send_to_agent.rssrc/ui/orchestration.rssrc/github/error.rssrc/app_shell.rssrc/state/prs_property_ops.rssrc/app_input/issue_prep.rssrc/lib.rssrc/state/form_build.rssrc/services/mod.rssrc/state/issues_property_ops.rssrc/runtime/gh_auth.rssrc/git_info/mod.rssrc/services/normalize.rssrc/local_command.rssrc/app_input/issue_prep_tests.rssrc/github/actions.rssrc/github/mod.rssrc/app_input/issue_cleanup.rssrc/app_input/issue_git_prep.rs
📚 Learning: 2026-07-08T06:45:09.102Z
Learnt from: acoliver
Repo: vybestack/llxprt-jefe PR: 149
File: src/state/prs_nav_ops.rs:487-488
Timestamp: 2026-07-08T06:45:09.102Z
Learning: In this Rust codebase, review `AppState` methods defined in `src/state/*` submodules for correct visibility. Because the `impl` is inside a (possibly private) `state::*` module, an inherent method cannot be accessed from outside that module unless its defining module allows it. If a method is called from modules outside `state` (e.g., `src/mouse_routing.rs`), it must be `pub`; using `pub(crate)` can still fail to compile with E0624 (“method is private”) when the containing submodule is declared as `mod ...;` (private) in `src/state/mod.rs` rather than `pub mod ...;`. Before suggesting `pub(crate)`, confirm the submodule is exported as `pub` from `src/state/mod.rs`. As precedent, `pr_detail_max_scroll_offset` and `IssuesState::max_detail_scroll_offset` are `pub` because they’re called outside `state`.
Applied to files:
src/state/issues_tests_send_to_agent.rssrc/state/prs_property_ops.rssrc/state/form_build.rssrc/state/issues_property_ops.rs
🔇 Additional comments (20)
src/app_shell.rs (1)
267-268: LGTM!src/state/issues_property_ops.rs (1)
670-672: LGTM!src/state/issues_tests_send_to_agent.rs (1)
39-39: LGTM!src/state/prs_property_ops.rs (1)
642-644: LGTM!src/lib.rs (1)
16-17: LGTM!src/local_command.rs (1)
82-160: LGTM! Override-path validation, invalid-override error message, and PATH empty/empty-component filtering (previously flagged) all now correctly handled.src/github/error.rs (1)
9-33: LGTM!src/github/mod.rs (1)
72-81: LGTM!gh_command()now differentiatesNotFoundvsInvalidOverride(resolving the prior coarse-mapping concern), and all call sites consistently route through it.Also applies to: 180-198, 221-259, 266-323, 329-363, 393-425, 432-463, 470-505, 802-815, 963-987
src/github/actions.rs (1)
270-285: LGTM!src/services/mod.rs (1)
20-20: LGTM!src/services/normalize.rs (1)
9-28: LGTM!Also applies to: 74-96, 199-277
src/state/form_build.rs (1)
248-254: LGTM!src/app_input/issue_prep.rs (1)
213-229: LGTM!src/app_input/issue_cleanup.rs (1)
75-104: LGTM!src/app_input/issue_git_prep.rs (3)
532-552: 🩺 Stability & AvailabilityConfirm
LC_ALL=C/ English-stderr assumption still holds for the resolved Windowsgit.exe.
git_capturenow spawns the executable resolved byjefe::local_command::command(LocalTool::Git)instead of a bare"git"lookup. The existingLC_ALL=Cenv var and the comment noting stderr is parsed for the linked-worktree fallback (incheckout_and_pull) depend on git emitting English messages; please confirm Git for Windows (MSYS/mingw builds) honorsLC_ALLthe same way as Unix git, otherwise thecontains()stderr check could silently break for non-English Windows locales.
586-586: LGTM!
18-29: 📐 Maintainability & Code QualityNo issue:
validate_reclone_targetis re-exported on the next line.> Likely an incorrect or invalid review comment.src/app_input/issue_prep_tests.rs (1)
343-400: LGTM!src/ui/orchestration.rs (1)
148-154: LGTM!dev-docs/tmux-scenarios/windows-local-repository-prep.json (1)
1-26: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/services/normalize.rs`:
- Around line 54-55: Update the rooted-path determination around
windows_drive_prefix so a drive-relative prefix such as C: does not make the
path rooted; require the remaining path to begin with the platform’s root
separator for drive-rooted paths. Preserve parent components during
normalization and add a regression assertion that C:..\foo and C:foo are not
equivalent.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d8aa8001-6b58-477c-a40e-2fec87c02011
📒 Files selected for processing (5)
src/app_input/issue_cleanup.rssrc/git_info/mod.rssrc/local_command.rssrc/runtime/gh_auth.rssrc/services/normalize.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Native Windows (MSVC + psmux)
- GitHub Check: OpenCodeReview
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-04-04T21:05:37.792Z
Learnt from: acoliver
Repo: vybestack/llxprt-jefe PR: 36
File: src/github/mod.rs:537-542
Timestamp: 2026-04-04T21:05:37.792Z
Learning: In Rust, it’s acceptable for function parameters to be intentionally unused when their names start with an underscore (e.g., `_cursor: Option<&str>`). If the parameter name is underscore-prefixed, treat it as a deliberate forward-compatibility/API-consistency choice and do not raise an “unused parameter”/dead code warning during code review.
Applied to files:
src/runtime/gh_auth.rssrc/app_input/issue_cleanup.rssrc/services/normalize.rssrc/git_info/mod.rssrc/local_command.rs
📚 Learning: 2026-06-22T22:25:52.693Z
Learnt from: acoliver
Repo: vybestack/llxprt-jefe PR: 81
File: src/app_init.rs:11-13
Timestamp: 2026-06-22T22:25:52.693Z
Learning: In Rust, when you call a method that is defined on a trait (e.g., `spawn_session` from `jefe::runtime::RuntimeManager`) on a concrete type (e.g., `TmuxRuntimeManager`), ensure the defining trait is imported into scope (`use ...::RuntimeManager`) if it is not already in scope. Otherwise the call may fail with a “method not found” error because Rust’s method-call resolution requires the trait to be in scope.
Applied to files:
src/runtime/gh_auth.rssrc/app_input/issue_cleanup.rssrc/services/normalize.rssrc/git_info/mod.rssrc/local_command.rs
🔇 Additional comments (4)
src/git_info/mod.rs (1)
12-13: LGTM!Also applies to: 45-46, 203-215, 234-234, 258-258
src/runtime/gh_auth.rs (1)
16-16: LGTM!Also applies to: 29-30, 59-67
src/local_command.rs (1)
114-118: LGTM!Also applies to: 137-139
src/app_input/issue_cleanup.rs (1)
3-3: LGTM!Also applies to: 69-72, 128-153, 159-184
1 participant
Summary
PATH/PATHEXTbehavior andJEFE_GIT_BIN/JEFE_GH_BINoverridesghvalue as a distinct argv item, preserving spaces, Unicode, refs, prompts, and paths without a local shellgit clean/reset --hardcleanup with enumerated untracked-file deletion and per-path tracked restoration, preserving.jefeand.llxprtmetadata and surfacing Windows file-lock failuresTest-first evidence
Verification
cargo fmt --all --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo build --workspace --all-features --lockedcargo test --workspace --all-features --lockedAll passed on native Windows.
Fixes #261