Add remote support for git operations#12230
Conversation
MaggieShan
force-pushed
the
maggs/add-remote-git-ops-support
branch
from
6864634 to
b42e2dd
Compare
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR enables the code-review git operations dialog for remote repositories by adding host-scoped git RPCs, routing commit/push/create-PR/PR-info through the remote diff model, and carrying per-file change metadata through the proto/model layers.
Concerns
- The Create PR dialog now uses branch diff metadata that includes uncommitted and untracked working-tree changes, but the actual PR creation path still targets the pushed branch, so the dialog can show files that will not be included in the PR.
- This is a user-facing behavior change and the PR description does not include screenshots or a screen recording. For this user-facing change, please include screenshots or a screen recording demonstrating remote commit, push/publish, create PR, and view PR working end to end.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR enables code-review git operations for remote sessions by adding host-scoped git RPCs, wiring remote commit/push/create-PR flows through DiffStateModel, and carrying per-file change metadata into the git dialogs. The PR includes Loom visual evidence.
Concerns
- The new shared git-operation-in-progress guard still assumes
.gitis a directory, so it misses merge/rebase/cherry-pick/index-lock sentinels in linked worktrees where.gitis a pointer file. - A remote-manager comment overstates the privacy boundary for AI commit-message generation; the diff is sent to Warp's AI endpoint, even though it is not returned over the remote-server response.
Security
- The AI commit-message documentation should clarify where diff content is sent so future changes do not rely on an incorrect data-boundary assumption.
Verdict
Found: 0 critical, 1 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| /// daemon-side execution-time check. | ||
| #[cfg(feature = "local_fs")] | ||
| pub fn git_operation_in_progress(repo_path: &Path) -> bool { | ||
| let git_dir = repo_path.join(".git"); |
There was a problem hiding this comment.
.git is a directory; linked worktrees store .git as a pointer file, so MERGE_HEAD, rebase state, and index.lock live in the real git dir and remote git ops can still run during an in-progress operation. Resolve the git dir first (for example via git rev-parse --git-dir or the gitdir: pointer) before checking sentinels.
MaggieShan
force-pushed
the
maggs/add-remote-git-ops-support
branch
from
9123428 to
ee569bf
Compare
kevinyang372
left a comment
kevinyang372
left a comment
There was a problem hiding this comment.
Nice! Didn't review too closely but we already aligned on the approach high-level
|
|
||
| /// Runs a commit chain on the remote host. The result arrives via | ||
| /// `DiffStateModelEvent::GitOpCompleted`. No-op for local repos. | ||
| pub(crate) fn git_commit_chain( |
There was a problem hiding this comment.
It's a bit weird design wise that we have this operation that only works for the remote case
How's it modeled locally? I would consider branching at the local callsites instead of here or at least change the naming so it's clearer this is remote only
| /// Issues an AI commit-message generation request on the remote host. The | ||
| /// result arrives via `DiffStateModelEvent::CommitMessageGenerated`. No-op | ||
| /// for local repos, which generate the message client-side in the dialog. | ||
| pub(crate) fn generate_commit_message( |
| repeated Commit unpushed_commits = 6; | ||
| optional string upstream_ref = 7; | ||
| optional PrInfo pr_info = 8; | ||
| // Field 9 (has_staged_changes) was removed once the commit dialog stopped |
There was a problem hiding this comment.
I am confused by this comment -- why do we need this?
| /// (see `handle_git_commit_chain`), so the SSH link carries one request/response | ||
| /// instead of the 2–3 a client-side chain would send. | ||
| #[allow(clippy::too_many_arguments)] | ||
| pub async fn git_commit_chain( |
There was a problem hiding this comment.
It's unclear what this name actually means in here and proto -- I would consider naming this commit_and_create_pr?
2 participants
Description
FileChangeEntrythroughDiffMetadataAgainstBaseandCommitto track per file diff stats that are rendered in the git dialogLinked Issue
APP-4512
Testing
https://www.loom.com/share/112c2add953147d7b6be857b5f655abf
./script/runAgent Mode
Changelog Entries for Stable
CHANGELOG-NEW-FEATURE: Git operations are now supported on remote sessions