Repository navigation
feat: all SSH operations just work, remove meta git setup-ssh - #20
Merged
Merged
Conversation
Three code paths now self-manage SSH multiplexing via GIT_SSH_COMMAND: 1. clone.rs: Establishes ControlMaster for meta repo host before initial clone, then for child repo hosts before parallel workers 2. update.rs: Same pattern before parallel clone of missing repos 3. lib.rs (raw git): Establishes masters and injects GIT_SSH_COMMAND into PlannedCommand env for parallel remote operations Key behaviors: - Respects existing user SSH config (ssh -O check detects active masters) - Falls back to serial execution if SSH setup fails - Never modifies ~/.ssh/config - Creates ~/.ssh/sockets with mode 700 if missing Removed: - meta git setup-ssh command (dispatch, help, registration) - ssh_pre_commands() approach (replaced by direct master establishment) - Interactive check_and_fix_remotes() prompt (now warn-only in update) Implements [[tasks/clone-self-contained-ssh]] Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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:
WalkthroughSSH ControlMaster multiplexing is attempted for detected SSH repo URLs before cloning; when sockets are established a Changes
Sequence Diagram(s)sequenceDiagram
participant Updater as Updater
participant Meta as Meta Parser
participant SSHSetup as ssh_setup
participant Queue as CloneQueue
participant Worker as Clone Worker
participant Git as git process
Updater->>Meta: extract SSH URLs from meta
Updater->>SSHSetup: establish_ssh_masters([hosts])
SSHSetup-->>Updater: OurSockets(dir) / UserManaged / Failed
alt OurSockets(dir)
Updater->>SSHSetup: git_ssh_command(dir)
SSHSetup-->>Updater: GIT_SSH_COMMAND
Updater->>Queue: create & seed queue
Queue->>Updater: peek_urls()
Updater->>SSHSetup: establish_ssh_masters(queued hosts)
Updater->>Worker: clone_with_queue(..., ssh_cmd)
Worker->>Git: spawn git clone (env: GIT_SSH_COMMAND)
else Failed
Updater->>Updater: set parallel = 1, ssh_cmd = None
Updater->>Worker: clone_with_queue(..., None)
Worker->>Git: spawn git clone (default SSH)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR automates SSH ControlMaster setup so Key changes:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| src/ssh_setup.rs | Core SSH multiplexing module. Fixes from prior rounds applied cleanly: resolved_user() now uses $USER/$LOGNAME for userless targets, SshMasters tristate properly distinguishes UserManaged from Failed, ControlPath is single-quoted, ConnectTimeout=10 added to git_ssh_command(), StrictHostKeyChecking removed. Well-tested with 11 unit tests covering all URL formats. |
| src/ssh.rs | New utility module for SSH URL discovery and remote mismatch warnings. Remote mismatch fix suggestions now single-quote path and URL. Non-interactive design (warn-only) is correct for automated context. Minor: the condition on line 56 (&&) effectively only skips fully-absent repos, but get_remote_url gracefully returns None for non-git dirs so behavior is correct. |
| src/clone.rs | SSH masters established in two phases: once for the meta-repo host before its initial clone, then again for child-repo hosts found in the queue. The Failed guard correctly avoids serializing when at least one socket already works. Minor: when the second establish_ssh_masters() returns UserManaged, the ssh_cmd from the first call is not cleared, so child workers may inject a GIT_SSH_COMMAND pointing to a socket dir that has no socket for their host – works, but bypasses the user's existing multiplexing on those hosts. |
| src/update.rs | SSH multiplexing correctly added before parallel cloning. Remote URL mismatch warnings moved here from interactive setup-ssh and made non-interactive. Fallback-to-serial logic is consistent with clone.rs. Orphaned-repo warnings retained. |
| src/lib.rs | Raw git command path now establishes SSH masters for parallel remote ops and injects GIT_SSH_COMMAND per PlannedCommand. SSH_MAX_SESSIONS cap and spawn stagger added via FullPlan. setup-ssh removed from help and from the help-text test. Test assertions look correct (third assert now checks !contains("meta git setup-ssh")). |
| src/clone_worker.rs | ssh_cmd parameter threaded cleanly through the worker pool and into each git clone subprocess via GIT_SSH_COMMAND. Worker termination logic (active-count + condvar) handles the queue-growing-during-clone case correctly. |
| src/main.rs | setup-ssh command cleanly removed from the plugin command registry and help sections. No regressions. |
Sequence Diagram
sequenceDiagram
participant U as User
participant CLI as meta git clone/update
participant SSHSetup as ssh_setup::establish_ssh_masters
participant SSH as SSH Master Process
participant Git as git subprocess
participant Remote as Git Remote (SSH)
U->>CLI: meta git clone <url>
CLI->>SSHSetup: establish_ssh_masters([meta-repo-url])
SSHSetup->>SSH: ssh -O check (detect user masters)
alt user master exists
SSH-->>SSHSetup: success → UserManaged
SSHSetup-->>CLI: UserManaged (no GIT_SSH_COMMAND)
else no user master
SSHSetup->>SSH: ssh -fNM -o ControlMaster=auto -o ControlPath=... -o ControlPersist=600
SSH->>Remote: TCP + auth (blocks until ready)
SSH-->>SSHSetup: exit 0 (master forked to background)
SSHSetup-->>CLI: OurSockets(dir)
CLI->>Git: git clone [GIT_SSH_COMMAND=ssh -o ControlPath=...]
end
Git->>SSH: multiplexed channel (reuses master)
SSH->>Remote: clone data
Remote-->>Git: clone complete
Git-->>CLI: meta repo cloned
CLI->>SSHSetup: establish_ssh_masters(child-repo-urls)
SSHSetup-->>CLI: OurSockets / UserManaged / Failed
CLI->>CLI: spawn worker pool (parallel=4 or 1)
loop for each child repo
CLI->>Git: git clone [GIT_SSH_COMMAND injected if OurSockets]
Git->>SSH: multiplexed channel
SSH->>Remote: clone data
Remote-->>Git: done
end
CLI-->>U: Clone complete (N repos)
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/clone.rs
Line: 173-174
Comment:
**`UserManaged` arm doesn't clear the meta-repo `ssh_cmd`**
When the second `establish_ssh_masters` returns `UserManaged` (all child-repo hosts already have active user masters), the `ssh_cmd` value from the *first* call (for the meta-repo host) is left in place. That means `clone_with_queue` will still inject a `GIT_SSH_COMMAND` pointing to our socket dir for every worker, even on child-repo hosts that have user-managed sockets in a *different* location.
In the common case — all repos on the same host (e.g. `github.com`) — this never triggers: when our socket for that host already exists in the sockets dir, the second call returns `OurSockets`, not `UserManaged`. But when the meta-repo is on a different host from the child repos and the user already has active masters for the child-repo hosts, our injected `GIT_SSH_COMMAND` causes SSH to attempt to create *new* sockets (via `ControlMaster=auto`) in our dir rather than reusing the user's existing ones — quietly bypassing the user's multiplexing.
Consider clearing `ssh_cmd` when `UserManaged` is returned, so child workers fall back to SSH's own connection management:
```rust
ssh_setup::SshMasters::UserManaged => {
// User's own masters cover all child hosts — no injection needed.
ssh_cmd = None;
}
```
(If you want to keep our socket for the meta-repo host reachable to any child-repo workers that happen to share the same host, you could instead only clear when `ssh_cmd` was set for a *different* host — but `None` is the safe, simple default here.)
How can I resolve this? If you propose a fix, please make it concise.Reviews (5): Last reviewed commit: "fix: resolve socket_name to OS user for ..." | Re-trigger Greptile
Comment on lines
+109
to
+113
| // If no host needed our master (all had existing connections), return None | ||
| // so callers don't override GIT_SSH_COMMAND unnecessarily | ||
| if !any_needed_our_master { | ||
| debug!("All hosts have existing ControlMaster connections, no override needed"); | ||
| return None; |
There was a problem hiding this comment.
None return conflates "user masters exist" with "setup failed" — serializes when it should parallelize
establish_ssh_masters returns None for two distinct situations: (a) all hosts already have active user-managed ControlMaster connections, and (b) all host connections failed. Both callers (update.rs and lib.rs) treat either None as a failure and fall back to serial execution:
// update.rs – both None cases hit this branch
None => {
log::warn!("SSH multiplexing setup failed, falling back to serial cloning");
parallel = 1;
None
}
// lib.rs – both None cases produce sequential
} else {
// SSH setup failed — fall back to sequential
CommandResult::Plan(commands, Some(false))
}This means that users who already have SSH multiplexing configured in ~/.ssh/config — exactly the users who least need serialization — will get the slowest path, plus a misleading "SSH multiplexing setup failed" warning.
The PR description says "Respects existing user SSH config (ssh -O check detects active masters — no override)", but the runtime behavior is the opposite: existing masters trigger a serial fallback.
Consider distinguishing the two None outcomes, for example by returning an enum or a (Option<PathBuf>, bool /* user_masters_already_active */) tuple, and only serializing on genuine failures:
pub enum SshMasters {
OurSockets(PathBuf), // we set up masters; inject GIT_SSH_COMMAND
UserManaged, // all hosts already have active masters; run parallel, no override
Failed, // could not connect; fall back to serial
}Callers would then be:
match ssh_setup::establish_ssh_masters(&host_refs) {
SshMasters::OurSockets(dir) => Some(ssh_setup::git_ssh_command(&dir)),
SshMasters::UserManaged => None, // parallel OK, no GIT_SSH_COMMAND needed
SshMasters::Failed => { parallel = 1; None }
}Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ssh_setup.rs
Line: 109-113
Comment:
**`None` return conflates "user masters exist" with "setup failed" — serializes when it should parallelize**
`establish_ssh_masters` returns `None` for two distinct situations: (a) all hosts already have active user-managed ControlMaster connections, and (b) all host connections failed. Both callers (`update.rs` and `lib.rs`) treat either `None` as a failure and fall back to serial execution:
```rust
// update.rs – both None cases hit this branch
None => {
log::warn!("SSH multiplexing setup failed, falling back to serial cloning");
parallel = 1;
None
}
// lib.rs – both None cases produce sequential
} else {
// SSH setup failed — fall back to sequential
CommandResult::Plan(commands, Some(false))
}
```
This means that users who already have SSH multiplexing configured in `~/.ssh/config` — exactly the users who *least* need serialization — will get the slowest path, plus a misleading "SSH multiplexing setup failed" warning.
The PR description says "Respects existing user SSH config (`ssh -O check` detects active masters — no override)", but the runtime behavior is the opposite: existing masters trigger a serial fallback.
Consider distinguishing the two `None` outcomes, for example by returning an enum or a `(Option<PathBuf>, bool /* user_masters_already_active */)` tuple, and only serializing on genuine failures:
```rust
pub enum SshMasters {
OurSockets(PathBuf), // we set up masters; inject GIT_SSH_COMMAND
UserManaged, // all hosts already have active masters; run parallel, no override
Failed, // could not connect; fall back to serial
}
```
Callers would then be:
```rust
match ssh_setup::establish_ssh_masters(&host_refs) {
SshMasters::OurSockets(dir) => Some(ssh_setup::git_ssh_command(&dir)),
SshMasters::UserManaged => None, // parallel OK, no GIT_SSH_COMMAND needed
SshMasters::Failed => { parallel = 1; None }
}
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/clone_worker.rs (1)
31-42:⚠️ Potential issue | 🟠 MajorA fixed SSH command can't cover repos discovered mid-run.
Workers snapshot one immutable
GIT_SSH_COMMANDbefore any nested.metaexpansion. Ifqueue.mark_completed()later enqueues a repo on a new SSH host,clone_single_repo()has no way to pre-establish that host, so recursive fresh-machine clones fall back to a first-contact SSH path instead of the advertised preflight. Pass an SSH setup context (or establish fromtask.urllazily) rather than only a prebuilt string.Also applies to: 105-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/clone_worker.rs` around lines 31 - 42, The worker threads currently capture a single immutable Arc<String> ssh_cmd at spawn time, so repos discovered later (via queue.mark_completed()) can't get SSH preflight for new hosts; change the worker closure and clone_single_repo() usage to accept a dynamic SSH setup context or a lazy resolver instead of the prebuilt ssh_cmd string: pass either a clone of a shared SSHSetup/Resolver object or pass task.url into a function that ensures host preflight before cloning, and update calls that currently use ssh_cmd (including the worker closure where ssh_cmd is Arc::clone(&ssh_cmd) and the clone_single_repo(...) invocation) to call the lazy setup resolver so new hosts discovered mid-run are prepared on-demand.src/ssh.rs (1)
64-67:⚠️ Potential issue | 🟡 MinorUse the repository path in the remediation command.
git -Cexpects a filesystem path, but the warning stores/printsproject.name. Whenname != path(nested repos, aliases), the suggested command points at the wrong checkout. Carryproject.paththroughRemoteMismatchand use that here.Also applies to: 97-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ssh.rs` around lines 64 - 67, The remediation command currently uses project.name (a repo name) but git -C requires the filesystem path, so update the RemoteMismatch data structure to carry project.path (e.g., add a path: String field to RemoteMismatch), populate it where mismatches are created (replace usage in the mismatches.push at RemoteMismatch { name: project.name.clone(), ... } to also set path: project.path.clone()), and then change the remediation/print logic (the other occurrence around the 97-99 area where the suggested git -C command is built) to use RemoteMismatch.path instead of RemoteMismatch.name so the suggested git -C points at the actual checkout path. Ensure any constructors, pattern matches, or usages of RemoteMismatch are updated to handle the new path field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ssh_setup.rs`:
- Around line 40-45: The function establish_ssh_masters currently returns
Option<PathBuf> which conflates “no-op because user already has masters” and
“failure to set up multiplexing”; change its return type to a richer enum (e.g.
SshMultiplexSetup { Enabled(PathBuf), NotNeeded, Failed }) and update the
function body (including the logic around the second block mentioned at lines
~109-120) to return the correct variant in each case; then update all callers
(the ones in clone.rs, update.rs and lib.rs) to only fall back to serial
execution when the variant is Failed and to keep parallelism when NotNeeded or
Enabled. Ensure all match arms and usages are adjusted accordingly.
- Around line 16-18: The code currently hardcodes "git@{host}" and port 22 when
probing and creating SSH control sockets; update the flow to parse and carry the
SSH triple (user, host, port) through the relevant functions (e.g.,
has_existing_master, start_master, any probe/control_path helper) instead of
only host, then use those components to build the SSH target and control-path
consistently (respecting non-default user and port like
ssh://alice@example.com:2222) — parse the remote into user/host/port early, pass
the triple into has_existing_master and the master startup path, and construct
the control-socket name using the same %r@%h-%p semantics (or equivalent
formatted "{user}@{host}-{port}") and pass port with -p when invoking ssh.
In `@src/ssh.rs`:
- Around line 8-13: The code in src/ssh.rs currently synthesizes "github.com"
when meta config isn't found or fails to parse; instead, return an empty Vec so
callers (e.g., execute_raw_git_command) can skip SSH setup for HTTPS-only repos.
Update both early returns that use meta_core::config::find_meta_config(...) and
meta_core::config::parse_meta_config(...) to return vec![] (empty list) rather
than vec!["github.com".to_string()] — this change should also be applied to the
analogous returns around lines 22-25 that handle the same error cases.
In `@src/update.rs`:
- Around line 17-18: The current call to crate::ssh::warn_remote_mismatches(cwd)
runs only for cwd and should be moved so warnings run for every discovered meta
root: after building dirs_to_check, remove the existing call and iterate over
dirs_to_check (for dir in &dirs_to_check) calling
crate::ssh::warn_remote_mismatches(dir) for each entry so recursive updates
report mismatches for nested workspaces as well.
---
Outside diff comments:
In `@src/clone_worker.rs`:
- Around line 31-42: The worker threads currently capture a single immutable
Arc<String> ssh_cmd at spawn time, so repos discovered later (via
queue.mark_completed()) can't get SSH preflight for new hosts; change the worker
closure and clone_single_repo() usage to accept a dynamic SSH setup context or a
lazy resolver instead of the prebuilt ssh_cmd string: pass either a clone of a
shared SSHSetup/Resolver object or pass task.url into a function that ensures
host preflight before cloning, and update calls that currently use ssh_cmd
(including the worker closure where ssh_cmd is Arc::clone(&ssh_cmd) and the
clone_single_repo(...) invocation) to call the lazy setup resolver so new hosts
discovered mid-run are prepared on-demand.
In `@src/ssh.rs`:
- Around line 64-67: The remediation command currently uses project.name (a repo
name) but git -C requires the filesystem path, so update the RemoteMismatch data
structure to carry project.path (e.g., add a path: String field to
RemoteMismatch), populate it where mismatches are created (replace usage in the
mismatches.push at RemoteMismatch { name: project.name.clone(), ... } to also
set path: project.path.clone()), and then change the remediation/print logic
(the other occurrence around the 97-99 area where the suggested git -C command
is built) to use RemoteMismatch.path instead of RemoteMismatch.name so the
suggested git -C points at the actual checkout path. Ensure any constructors,
pattern matches, or usages of RemoteMismatch are updated to handle the new path
field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a788568-4502-4a4b-95f0-9eadfbff8d18
📒 Files selected for processing (7)
src/clone.rssrc/clone_worker.rssrc/lib.rssrc/main.rssrc/ssh.rssrc/ssh_setup.rssrc/update.rs
💤 Files with no reviewable changes (1)
- src/main.rs
- Return SshMasters enum instead of Option to distinguish UserManaged (all hosts have existing masters) from Failed (all connections failed). UserManaged preserves parallelism; only Failed falls back to serial. - Parse full SSH target (user, host, port) from URLs instead of hardcoding git@host:22. Supports ssh://alice@host:2222/path. - Single-quote ControlPath in GIT_SSH_COMMAND to handle spaces in paths - Return empty vec from discover_ssh_hosts for HTTPS-only workspaces instead of synthesizing github.com - Fix duplicate assertion in help test to verify setup-ssh is absent - Run remote mismatch warnings per meta root in recursive mode Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
All review feedback addressed in bf031a7:
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ssh_setup.rs`:
- Around line 140-146: The code treats the presence of a socket file
(socket_exists_in) as a success even when has_existing_master returned false,
which can leave stale sockets counted as successes; update the logic in the
block handling socket_exists_in(&sockets_dir, target) so that you either verify
the socket points to an active master (call has_existing_master again or attempt
a lightweight connect) before setting any_succeeded = true and
any_needed_our_master = true, or remove the stale socket (unlink) and fall
through to create a new connection; ensure you reference socket_exists_in,
has_existing_master, any_needed_our_master, any_succeeded, sockets_dir, and
target when making the change.
In `@src/ssh.rs`:
- Around line 5-23: The docstring and behavior of discover_ssh_hosts are
mismatched and cause HTTPS-only workspaces to trigger an unnecessary serial
fallback; change discover_ssh_hosts to return Option<Vec<String>>
(Some(vec![...]) when SSH hosts found, Some(vec![]) when discovery succeeded but
no SSH hosts, and None on discovery error), update its docstring to reflect this
contract, and adjust callers (e.g., establish_ssh_masters and the logic in
lib.rs that interprets its result) to treat Some([]) as “no SSH hosts — skip SSH
setup and keep parallel execution” while only treating None as a discovery
failure that should trigger the fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a7b39b6b-405a-46e5-b6d4-3ce55be6efb5
📒 Files selected for processing (5)
src/clone.rssrc/lib.rssrc/ssh.rssrc/ssh_setup.rssrc/update.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ssh_setup.rs`:
- Around line 130-199: The loop currently only sets global flags
(any_needed_our_master / any_succeeded) so a single OurSockets return causes
callers to override user-managed masters for targets that didn't need our
socket; change the logic to record per-target state: create a collection (e.g.,
needed_our_master_targets: Vec<Target> or Vec<TargetId>) and push the current
target when socket_exists_in(...) or when cmd.status() succeeds, preserve
skipping for has_existing_master(target), then at the end use that collection to
decide returns (if needed_our_master_targets.is_empty() ->
SshMasters::UserManaged) and change SshMasters::OurSockets to carry the
sockets_dir plus the list of targets that actually need tool-managed sockets (or
add a new variant like SshMasters::PerTarget(sockets_dir, Vec<TargetId>));
update callers to consult that per-target list before setting GIT_SSH_COMMAND.
- Around line 98-125: Reorder establish_ssh_masters so it parses and
deduplicates SSH targets (using parse_ssh_target and the existing seen/key
logic) before calling meta_git_lib::ensure_ssh_sockets_dir, and if
targets.is_empty() return a no-op state (e.g., SshMasters::UserManaged or add
SshMasters::NotNeeded) instead of SshMasters::Failed; only proceed to create the
sockets dir and continue setup when there are one or more targets to manage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: df8a6098-c277-4299-bfa9-08076c3b3266
📒 Files selected for processing (1)
src/ssh_setup.rs
Contributor
Author
|
🤖 Agent starting ( |
- Return early with parallel=true when no SSH hosts found (HTTPS-only) - Return UserManaged instead of Failed when no SSH targets parsed - Move sockets dir creation after empty-targets check - Remove stale sockets before creating fresh masters - Fix docstring for discover_ssh_hosts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
Review Summary
Build: clippy clean, 67/67 tests pass (macOS) Confidence: 95% — the one gap is the mixed-target GIT_SSH_COMMAND override (T11), which is benign in practice (single-host workspaces) but worth a follow-up for correctness. 🤖 Generated with Claude Code |
|
The Let me trigger a fresh review against the latest commit to confirm everything looks good end-to-end. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib.rs`:
- Around line 138-140: The current mapping that builds SCP-style URLs with
format!("git@{h}:") loses user and port info extracted earlier; update the flow
so discover_ssh_hosts (or the caller) returns full SSH URLs or a parsed struct
(e.g., user, host, port, path) instead of bare hostnames, and pass those full
URLs/structs into establish_ssh_masters rather than reconstructing with
git@host: in the urls/ url_refs creation; specifically modify discover_ssh_hosts
/ meta_git_lib::extract_ssh_host to return the full URL or tuple and replace the
code that builds urls from hosts (the hosts variable, the urls mapping and
url_refs) so it forwards the original full SSH connection information (including
user and port) to establish_ssh_masters.
In `@src/ssh_setup.rs`:
- Around line 42-46: The current host/port parsing uses
host_port.split_once(':') and p.parse::<u16>().unwrap_or(22) which silently
falls back to 22 on malformed ports; change this to attempt parse and on Err
emit a warning (e.g., via your logger or eprintln!) that includes the invalid
port string and the original host_port, then fall back to 22—locate the tuple
assignment building (host, port) and replace the unwrap_or behavior with
explicit match/if let so you can log the parse failure before using the default.
- Around line 153-167: The ControlPath argument is not quoted when building the
master ssh command, so paths with spaces will break; update the code that builds
control_path/Command::new("ssh") (the variable control_path and the arg
currently passed as &format!("ControlPath={control_path}")) to quote or properly
escape the path the same way git_ssh_command does (e.g., wrap the ControlPath
value in single quotes or perform proper shell-escaping) so the ControlPath
option works when sockets_dir contains spaces or special characters.
In `@src/ssh.rs`:
- Around line 16-22: discover_ssh_hosts currently reduces repo entries to bare
hostnames via meta_git_lib::extract_ssh_host, which loses user and port info
that later causes establish_ssh_masters to synthesize incorrect git@host: URLs;
change discover_ssh_hosts to return either the full original repository URL
strings or a small struct (e.g., {user, host, port, original}) instead of just
hostname, update meta_git_lib usage to extract a structured result if available
(or skip extraction and forward the raw repo string), and update
establish_ssh_masters signature and all call sites to accept that full
URL/struct so callers can use the original user and port when constructing SSH
endpoints.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f2ba47aa-570e-42aa-b835-f743fb2f2ace
📒 Files selected for processing (3)
src/lib.rssrc/ssh.rssrc/ssh_setup.rs
Contributor
Author
|
🤖 Agent starting ( |
…ove StrictHostKeyChecking override - Rename discover_ssh_hosts to discover_ssh_urls to return full SSH URLs instead of just hostnames, preventing loss of user and port information when passed to establish_ssh_masters (T12, T15) - Quote ControlPath in master startup to handle spaces in paths, consistent with git_ssh_command (T14) - Add warning log when falling back to port 22 on invalid port (T13) - Remove StrictHostKeyChecking=accept-new to respect user's own SSH host-key verification policy (T16) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nd::new The ControlPath in establish_ssh_masters doesn't need single-quoting because Command::new bypasses the shell. Spaces in paths are handled correctly as a single argument. This is different from git_ssh_command() where GIT_SSH_COMMAND IS evaluated by the shell and needs quotes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
Review fixes pushed
Build status: clippy clean, 67/67 tests passing Confidence: 95% — SSH end-to-end behavior can't be unit-tested, but URL parsing, deduplication, and control flow are well-covered. 🤖 Generated with Claude Code |
|
The 95% confidence is the honest number here. Let me trigger a fresh incremental review against ✅ Actions performedReview triggered.
|
Contributor
Author
Review Comment Resolution SummaryAll 5 unresolved review threads have been addressed and resolved. No new commits were needed — all issues were already fixed in prior commits (bf031a7, 947cac7).
Build Status
Confidence: 100%Code review across all 7 changed files found no additional issues. The SSH multiplexing implementation is clean, well-tested, and handles edge cases (HTTPS-only workspaces, stale sockets, user-managed masters, custom ports/users) correctly. 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ssh_setup.rs`:
- Around line 145-151: The current logic unlinks any socket found by
socket_exists_in(&sockets_dir, target) without verifying whether the socket is a
live tool-managed master; change this to run an SSH control check for the
computed socket_path (the same path built from
sockets_dir.join(format!("{}@{}-{}", target.user, target.host, target.port)))
using something like std::process::Command to invoke ssh -S <socket_path> -O
check and inspect the exit status/output; only call
std::fs::remove_file(&socket_path) if the ssh check indicates the control master
is not active (non-zero exit or specific error), otherwise leave the socket in
place so an existing live master (created by meta) is reused.
- Around line 37-40: The current logic forcibly substitutes a missing SSH
username with "git" by setting (user, host_port) = ("git".to_string(),
user_host), which changes the intended destination; instead preserve the absence
of an explicit user and only add "user@" when one was actually provided. Update
the parsing around user_host (the let (user, host_port) = ... block) to keep
user as an Option<String> (or otherwise track whether a username was present)
and stop defaulting to "git"; change all downstream uses that build probe
strings, socket keys, or callouts (the code paths that previously used user and
host_port, including the similar block at the 56-60 region) to conditionally
prepend "user@" only when the Option is Some, otherwise use host_port/original
destination unchanged. Ensure the socket key and probe string logic uses the
preserved optional username so ssh://host/path and host:path remain unresolved
by local username/config rather than being overridden by "git".
In `@src/ssh.rs`:
- Around line 29-32: RemoteMismatch currently carries only a label (name) so
remediation text uses the wrong directory for git commands; add a new field
(e.g., path: String) to the RemoteMismatch struct and update all places that
construct RemoteMismatch (wherever RemoteMismatch { name, expected, actual } is
created) to pass the configured repo path instead of or in addition to name,
then change any code that renders the suggested command (the string that
currently interpolates name into "git -C {name} remote set-url ...") to
interpolate the new path field; ensure all call sites and pattern matches for
RemoteMismatch are updated to handle the new field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 23e5fc40-7855-47f4-97db-53bdfd241f43
📒 Files selected for processing (3)
src/lib.rssrc/ssh.rssrc/ssh_setup.rs
Contributor
Author
|
🤖 Agent starting ( |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/ssh.rs (1)
29-32:⚠️ Potential issue | 🟠 MajorUse the configured repo path in the mismatch fix command.
The remediation text uses
git -C {m.name}, butnameis only a label. When a repo lives in a custom or nested directory, the printed fix points at the wrong working tree and can update the wrong remote. Carryproject.paththroughRemoteMismatchand render that instead.♻️ Minimal fix
pub(crate) struct RemoteMismatch { pub name: String, + pub path: String, pub expected: String, pub actual: String, } @@ mismatches.push(RemoteMismatch { name: project.name.clone(), + path: project.path.clone(), expected: expected_url.clone(), actual: actual_url, }); @@ - eprintln!(" git -C {} remote set-url origin {}", m.name, m.expected); + eprintln!(" git -C {} remote set-url origin {}", m.path, m.expected);Also applies to: 53-67, 97-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ssh.rs` around lines 29 - 32, RemoteMismatch currently holds name/expected/actual but the remediation command uses name (a label) instead of the repo working-tree path; update RemoteMismatch to include a path field (e.g., project.path) and propagate that value wherever RemoteMismatch instances are constructed, then change any rendering or fix-text generation that uses m.name for git -C to use the new m.path instead (adjust in the code that builds the mismatch message and any places referencing RemoteMismatch like the remediation formatter so fixes point at the correct working tree).src/ssh_setup.rs (2)
74-84:⚠️ Potential issue | 🟠 MajorProbe tool-managed sockets before treating them as stale.
has_existing_master()only sees masters discoverable through the user's SSH config. A live master from a previousmetarun insockets_dirwill still fail that check, hit this branch, and get unlinked, which defeats cross-run reuse and leaves the old process alive untilControlPersistexpires. Verify the exactsocket_pathwithssh -S <path> -O check <target>before removing it.Also applies to: 145-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ssh_setup.rs` around lines 74 - 84, has_existing_master() only checks masters via SSH config and may wrongly unlink tool-managed socket files; update the check to probe the exact socket path with ssh -S <socket_path> -O check <user@host> before treating it as stale. Modify has_existing_master (and the duplicate logic around the other block) to determine the tool-managed socket_path (from sockets_dir/name), and run Command::new("ssh") with args including "-S", socket_path, "-O", "check", and the target string; treat the master as existing if that command succeeds and only unlink the file when the socket-specific check fails.
23-26:⚠️ Potential issue | 🟠 MajorDon't force
gitwhen the remote omits a username.Defaulting
ssh://host/pathandhost:pathtogitrewrites valid remotes. That changes the destination used byssh -O check, socket naming, and master startup, so SSH-config-drivenUserresolution and existing masters can be missed or broken. Keep the user optional and only prependuser@when it was explicitly present.Also applies to: 33-41, 54-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ssh_setup.rs` around lines 23 - 26, SshTarget currently forces a username by using user: String; change it to user: Option<String> (update the struct SshTarget) and update all code that constructs SSH destinations, socket names, and master/startup checks to only prepend "user@" when user.is_some() (e.g., in Display/to_string implementations, socket_name generation, and any start_master/check_master helpers that reference SshTarget). Also adjust parsing/FromStr logic that creates SshTarget so it leaves user as None when the remote omits a username, and update any call sites that assumed a non-empty user to handle Option<String>.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/ssh_setup.rs`:
- Around line 74-84: has_existing_master() only checks masters via SSH config
and may wrongly unlink tool-managed socket files; update the check to probe the
exact socket path with ssh -S <socket_path> -O check <user@host> before treating
it as stale. Modify has_existing_master (and the duplicate logic around the
other block) to determine the tool-managed socket_path (from sockets_dir/name),
and run Command::new("ssh") with args including "-S", socket_path, "-O",
"check", and the target string; treat the master as existing if that command
succeeds and only unlink the file when the socket-specific check fails.
- Around line 23-26: SshTarget currently forces a username by using user:
String; change it to user: Option<String> (update the struct SshTarget) and
update all code that constructs SSH destinations, socket names, and
master/startup checks to only prepend "user@" when user.is_some() (e.g., in
Display/to_string implementations, socket_name generation, and any
start_master/check_master helpers that reference SshTarget). Also adjust
parsing/FromStr logic that creates SshTarget so it leaves user as None when the
remote omits a username, and update any call sites that assumed a non-empty user
to handle Option<String>.
In `@src/ssh.rs`:
- Around line 29-32: RemoteMismatch currently holds name/expected/actual but the
remediation command uses name (a label) instead of the repo working-tree path;
update RemoteMismatch to include a path field (e.g., project.path) and propagate
that value wherever RemoteMismatch instances are constructed, then change any
rendering or fix-text generation that uses m.name for git -C to use the new
m.path instead (adjust in the code that builds the mismatch message and any
places referencing RemoteMismatch like the remediation formatter so fixes point
at the correct working tree).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8bd18f88-fb05-466b-8240-33d459086dc7
📒 Files selected for processing (3)
src/lib.rssrc/ssh.rssrc/ssh_setup.rs
… use repo path - T17: Make SshTarget.user optional so SSH resolves omitted users from config or current OS user instead of forcing "git" - T18: Probe existing sockets with `ssh -O check -S` before removing; reuse live sockets from previous runs instead of unlinking them - T19: Add `path` field to RemoteMismatch so `git -C` uses the configured project path, not just the name - T20: Add ConnectTimeout=10 to git_ssh_command for consistency with master setup - T21: Single-quote path and URL in fix suggestion to handle spaces Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
Review fixes — 8600840
Build: clippy clean, 71/71 tests pass |
|
Tip For best results, initiate chat on the files or code changes.
The 71/71 test count (up from 67) and the four new targeted tests for the ✏️ Learnings added
🧠 Learnings used |
Contributor
Author
|
🤖 Agent starting ( |
When a URL omits a user, SSH expands %r to the OS user (e.g. alice), but socket_name() was returning _@host-port. This mismatch meant socket_exists_in() never found sockets for userless targets, and stale-socket cleanup targeted the wrong path. Use $USER/$LOGNAME as the fallback to match SSH's %r resolution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
Review SummaryFixes
Build Status
Confidence: 98%Minor non-blocking notes from dimensional review:
No other issues found. Code is production-ready. 🤖 Generated with Claude Code |
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
1 participant
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
meta git clone,meta git update, and all parallel remote git commands now self-manage SSH multiplexing viaGIT_SSH_COMMANDwith explicit-oflagsssh -O checkdetects active masters — no override)~/.ssh/configmeta git setup-sshcommand entirelymeta git update(non-interactive, warn-only)Three code paths fixed
clone.rs: SSH masters established for meta repo host (before initial clone) and child repo hosts (before parallel workers)update.rs: Same pattern before parallel clone of missing reposlib.rs(raw git): Masters established andGIT_SSH_COMMANDinjected intoPlannedCommand.envfor all parallel remote operationsBreaking change
meta git setup-sshis removed. Users who relied on it get the behavior automatically — no migration needed.Dependencies
Requires companion PR in
meta_git_libforensure_ssh_sockets_dir()andpeek_ssh_hosts().Test plan
cargo test -p meta_git_cli— all 61 tests passcargo check --workspace— clean compilemeta git cloneon fresh machine with no SSH configmeta git push --parallelon fresh machine🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Removed