yes, start implementing, make a new branch, make meaningful commits, and add tests as you go, when it makes sense create tests first

I’m reviewing PR #53 against the current codebase and will compare its approach to what git-sync already does around auth, remotes, and process execution before I give you a recommendation on whether to salvage it or restart.

PR #53 is already merged on May 13, 2026 and it’s about force semantics, not SSH. I’m checking the repo history and open PRs/issues for the actual SSH work so I can give you the takeover recommendation on the right branch.

I found the SSH work: it’s open draft PR #39 (support-ssh-auth) by pranavmangal, last updated May 7, 2026. I’m pulling its diff and comparing it to today’s main so the recommendation is based on mergeability and design fit, not just surface changes.

The draft is substantial enough to reuse pieces, but the shape matters more than the line count here. I’m checking the review trail and the current transport/auth layering so I can separate “useful groundwork” from “architecture you’ll just end up rewriting.”

I have the prior review comments now. The key blocker was not “general mess”; it was a specific transport model mismatch: the draft treats SSH like one long-lived session, while git-sync’s actual flows require multiple logical RPCs per endpoint. I’m checking the rest of the code for how much of that review is still current and what the smallest viable SSH design looks like now.

I’ve confirmed the existing review still stands against current main: the code still does multi-step source/target RPCs, especially under protocol v2 and batching, so a reusable single SSH process would still be the wrong model today. I’m checking one more thing now: whether the repo already has patterns for shelling out or SSH config handling that would change the implementation recommendation.

#53 is the wrong PR for SSH. PR #53 was merged on May 13, 2026 and is the force-flags work. The SSH draft is PR #39, feat: Add support for SSH authentication, still open/draft, last updated May 7, 2026, and GitHub currently marks it DIRTY for merge state.

I would not continue that branch as-is. I would continue the idea, from current main, and selectively reuse parts of the diff.

Why:

• The core abstraction is sound: introducing a transport interface instead of hard-wiring HTTP is the right direction, and most of the strategy/test signature churn is mechanical.
• The actual SSH transport design in PR #39 is wrong for this codebase. git-sync does multiple RPCs per endpoint in normal flows, especially protocol v2 and batching. You can see that in internal/gitproto/refs.go, where v2 does discovery and then ls-refs, and in the bootstrap/incremental/replicate strategies, which can make repeated fetch/push calls. A single long-lived ssh process per connection will break on the second RPC.
• Your earlier review comments still hold up against current main: per-RPC process lifetime, context handling, no forced git@, and explicit behavior for missing stats/progress are still the real issues.
• The branch is stale relative to main and overlaps active files like internal/syncer/syncer.go, internal/gitproto/push.go, and internal/gitproto/refs.go, so taking it over directly means you pay both the rebase cost and the transport rewrite cost.

My recommendation:

• Start a new branch from current main.
• Reuse the PR as reference for the interface extraction and test updates.
• Rewrite the SSH implementation around one ssh exec per logical RPC.
• Keep auth split clean: HTTP auth stays in internal/auth/auth.go; SSH should mostly rely on the local ssh binary, agent, and user config.
• Ship v1 without SSH throughput/progress if needed, but warn clearly when --progress or --show-stats is used with SSH.

Feasibility review: yes, this is worth doing.

• The repo is already close structurally: endpoint parsing already understands SSH-shaped URLs in tests, and the transport seam is small.
• The MVP is clear: support ssh:// and SCP-like URLs, git-upload-pack/git-receive-pack, protocol v1/v2, and user-local SSH config/agent behavior.
• The main cost is testing. You need one real integration path for v2 over SSH, ideally via an ssh shim or local sshd; otherwise this will regress silently.

So: take over the feature, but not by finishing PR #39 in place. Use it as scaffolding, not as the branch to merge.

If you want, I can turn this into a concrete implementation plan for a fresh SSH PR with the exact file-by-file steps.

SSH Plan

I’d structure this as a fresh PR from main, in 6 steps.

1. Define the transport seam cleanly. Use a gitproto.Conn interface like the draft did, but keep the constructor shape idiomatic: newConn(...) (gitproto.Conn, error). Refactor the existing HTTP code into HTTPConn without changing behavior first. That gives you a no-op preparatory commit you can merge/test independently.

2. Implement SSH as per-RPC process execution. Do not model SSH as a persistent session. Each RequestInfoRefs and each PostRPCStreamBody should spawn its own ssh ... git-upload-pack or git-receive-pack process. Pass GIT_PROTOCOL=version=2 for v2 calls. Honor context cancellation per call.

3. Keep SSH auth/config intentionally thin. Rely on the local ssh binary for agent keys, ~/.ssh/config, host aliases, IdentityFile, and User. Do not inject a default git@ username unless the URL explicitly carries one. Start with no custom SSH option surface unless you find a real need.

4. Decide v1 stats/progress behavior explicitly. For the first pass, I would not block on byte-accurate SSH stats. Instead:

• Detect SSH endpoints in session setup.
• If --progress or --show-stats is enabled, emit a one-line warning that SSH transport does not yet support byte-counted progress/throughput. That keeps scope contained and avoids fake zero-byte stats.

1. Add real SSH integration coverage. This is the gating item. Add at least:

• Source over SSH, protocol v2, end-to-end ref discovery + fetch.
• Target over SSH, end-to-end push.
• A multi-RPC path, not just one fetch and one push.
• Context cancellation or failing SSH command behavior. A local ssh shim is probably the fastest route unless you want a test sshd.

1. Document the operational constraints. Add short docs covering:

• Supported SSH URL forms.
• Reliance on local ssh config and agent.
• Host key behavior and known_hosts.
• --progress / --show-stats limitation if you defer full support.

Suggested PR shape

I’d split it into 3 commits:

1. gitproto: abstract HTTP transport behind Conn interface
2. gitproto: add SSH transport via per-RPC ssh exec
3. docs/tests: SSH coverage and user-facing limitations

Decision points

Before implementation, I’d lock these down:

• Support both ssh://host/path.git and git@host:path.git: yes.
• Support progress/stats in v1: warn only.
• Use external ssh instead of Go-native SSH library: yes, for config compatibility and lower auth complexity.

If you want, I can turn this into a checklist mapped to concrete files in this repo.

Agreed. Your pushback is right on the important parts, especially stats/progress and test placement.

I’d tighten the plan to this:

Revised Plan

1. gitproto: extract the transport seam without behavior change.

• Introduce gitproto.Conn.
• Rename current HTTP transport to HTTPConn.
• Keep helper shape consistent: either free functions taking Conn, or interface methods plus thin wrappers. I’d prefer:
  • RequestInfoRefs(ctx, conn, ...)
  • PostRPC(ctx, conn, ...)
  • PostRPCStream(ctx, conn, ...)
  • Interface only requires the primitive streaming operation(s) plus endpoint/progress access.
• Add/update unit tests in this commit so the refactor is mechanically safe.

1. Add SSHConn with per-RPC ssh exec.

• One process per RequestInfoRefs / PostRPCStreamBody.
• Use exec.CommandContext.
• No default git@; honor explicit URL user only, otherwise let ssh config decide.
• Keep BatchMode=yes.
• Fail clearly if ssh is not on PATH.
• Add unit tests for command construction, cancellation, stderr/error surfacing, and multi-RPC reuse.

1. Add SSH byte counting at the stream boundary.

• Wrap stdin/stdout for per-side byte counts, mirroring the HTTP counting intent.
• If this is clean, --stats, --show-stats, and --progress should just work.
• Only fall back to warnings if the implementation turns out messier than expected.
• If warning fallback is needed, warn for --stats, --show-stats, and --progress.

1. Add integration coverage using the existing backend test shape.

• Reuse the pattern from internal/syncer/git_http_backend_test.go.
• Prefer a local ssh shim that dispatches to real git-upload-pack / git-receive-pack.
• Cover:
  • source over SSH with protocol v2
  • target over SSH push
  • a multi-RPC flow
  • cancellation / failing ssh path

1. Document the behavior and explicit non-goals.

• Supported URL forms: ssh:// and SCP-like git@host:path.
• Decide and document whether git+ssh:// is supported.
• State reliance on local ssh config, agent, and known_hosts.
• Note BatchMode=yes and first-contact host key implications.
• Defer GIT_SSH_COMMAND / --ssh-command unless there is immediate product pressure.

Commit split

1. gitproto: extract transport interface and rename HTTPConn
2. gitproto: add SSH transport via per-RPC ssh exec
3. syncer/tests: add SSH counting and integration coverage
4. docs: document SSH behavior and limitations

Decisions to lock before coding

• BatchMode=yes: yes.
• git+ssh://: explicitly decide before implementation.
• GIT_SSH_COMMAND override: defer for now.
• Missing ssh binary: fail early in session setup.
• Credit/coordination: comment on PR #39 before opening the replacement PR and credit the prior draft in commit message/trailer.

This is the version I’d execute. If you want, I can turn it into a file-level checklist next.

Agreed.

I’d lock those three points like this before touching code:

1. SSH counting gets a fixed spike budget. I’ll try the counting wrapper first, with a hard cap of 1-2 hours. If it is not cleanly drop-in by then, I’ll switch to the warning path and keep the PR moving. No open-ended “almost there” detour.

2. Counting and integration tests become separate commits. Revised split:

3. gitproto: extract transport interface and rename HTTPConn

4. gitproto: add SSH transport via per-RPC ssh exec

5. syncer: add SSH byte counting or syncer: warn that SSH stats/progress are unsupported

6. tests: add SSH integration coverage

7. docs: document SSH behavior and limitations

8. git+ssh:// gets decided immediately. I’ll check what transport.ParseURL does and then either:

• support it if it already normalizes cleanly, or
• reject it explicitly with a clear error.

That gives a fully pinned execution plan with no scope ambiguity left. If you want, I can start implementing from that plan now.

I’m starting from main, creating a fresh SSH branch, and I’ll land this as a sequence of reviewable commits with tests alongside each step rather than one large diff.

Branch creation hit a sandbox restriction on .git/refs. I’m requesting permission to create the branch and make commits normally so I can keep the work in a clean commit series instead of leaving changes uncommitted.

The branch is in place. I’m validating one implementation decision up front now: how transport.ParseURL handles SSH URL forms, so the code and docs can be explicit instead of carrying that ambiguity into the transport layer.

I’ve pinned one unresolved behavior: transport.ParseURL accepts git+ssh:// as a distinct git+ssh scheme, so I’ll support it rather than special-case reject it. Next I’m landing the refactor-only transport seam with its test updates as the first commit.

I’ve got the refactor surface mapped now. The only place that needs transport-specific handling in the first commit is bootstrap’s GitHub preflight, because it reaches into HTTP client details directly; everything else can stay transport-agnostic through the interface.

I’m making the transport refactor as a behavior-preserving change first: introduce the interface, rename HTTP transport, and push the mechanical signature updates through the strategies and tests before adding any SSH-specific code.

The code compiles conceptually on the transport side now, but the tree still has the expected mechanical fallout: tests and strategy fakes still refer to the old concrete *Conn type. I’m sweeping those next so the first commit stays pure refactor instead of mixing in new behavior.

The refactor sweep is done. I’m formatting and running a focused test slice now before I commit it, so the first commit is proven as a pure structural change rather than “refactor plus latent breakage.”

The focused test run is still in flight. If this comes back clean, I’ll commit the seam refactor immediately and move to the SSH transport itself; if not, I’ll fix the fallout before I touch any new behavior.

There’s one real code fallout left from the refactor: a bootstrap test still tries to construct the old concrete gitproto.Conn. After that, the remaining failure is the sandbox blocking local test listeners, so I’ll rerun the transport tests with approval once the code compiles cleanly.

I fixed the last compile-level fallout from the seam change. I’m doing a non-executing go test -run '^$' pass now to compile the packages and tests without tripping the sandbox’s listener restriction, then I’ll escalate only the transport test run that needs local sockets.

The seam refactor is green. I’m committing it now as an isolated transport-abstraction change, then I’ll start the SSH transport commit on top with tests first where it’s practical.

I’m moving to behavior now. The SSH transport commit will be per-RPC from the start, with tests around command construction and repeated calls, so I don’t accidentally recreate the draft’s single-process bug and then have to back it out.

The SSH transport file is in, but I want to clean one thing immediately before it spreads: I overbuilt the error-joining helper. I’m simplifying that to standard library errors.Join and then wiring the syncer to instantiate SSH for ssh and git+ssh URLs.

I caught one transport detail before it became a regression: ssh://host:port/repo.git needs -p, while SCP-style URLs do not. I’m wiring that into the transport now and then adding the SSH unit tests around real command invocation via a temp shim.

I’m tightening cancellation semantics before the first SSH test run. The transport already uses exec.CommandContext, but I want the surfaced error to preserve context deadline exceeded rather than just “signal: killed,” because that’s the useful contract for callers and tests.

One leftover helper reference slipped through the cleanup. I’ve fixed that and I’m rerunning the SSH unit slice now; this should tell me whether the per-RPC transport works before I widen the test surface.

The SSH test slice is taking longer than the earlier compile runs, which usually means one of the shim-driven process tests is hanging rather than failing fast. I’m polling it once more, and if it’s stuck I’ll isolate the hanging test before I commit anything.

I found the hang: RequestInfoRefs was starting the SSH process but never closing its stdin, so the shim sat in cat waiting forever. That’s the right kind of failure to catch now; I’m fixing it before rerunning the SSH slice.

The SSH unit tests are green now, including repeated RPCs and cancellation. I’m doing one broader compile pass before I commit this transport layer so I don’t leave any unrelated package with stale assumptions about newConn returning only HTTP.

Two commits are down: the transport seam and the actual per-RPC SSH transport. I’m on the scoped stats/progress spike now to see whether SSH byte counting is genuinely drop-in; if it isn’t clean quickly, I’ll switch to the explicit warning path and keep moving.

The counting spike is not “30 lines and done” in this codebase if I want it to preserve both per-side throughput and per-service stats shape cleanly. I’m taking the fallback path now: explicit SSH warning for --progress and --show-stats, with tests, so the feature keeps moving instead of getting stuck in instrumentation work.

I realized the warning was positioned too late in session setup: if source ref discovery fails first, the user never sees it. I’m moving the warning to fire immediately after SSH transport creation, which is both more correct and makes the test deterministic without needing a live SSH endpoint.

The warning tests are hanging, which means I still have one code path unexpectedly waiting on process I/O during session setup. I’m isolating the specific test instead of guessing, then I’ll either fix the wait path or collapse the test down to the pure helper if the setup path is too brittle.

The warning fallback is in and covered. I’m committing that separately now, then the last step is the end-to-end SSH integration coverage on top.

I’m on the last major step now: an end-to-end SSH integration test using a local ssh shim that dispatches to real git-upload-pack / git-receive-pack. That gives coverage for protocol v2 discovery, repeated RPCs on the same logical endpoint, and an actual push without needing a real sshd.

The first SSH integration run found a real protocol bug, not a test issue: protocol v2 over SSH emits the capability advertisement again on each fresh git-upload-pack exec, so my transport was handing that advertisement back to ls-refs/fetch as if it were the command response. I’m fixing that in the SSH transport by discarding the initial v2 advertisement on per-RPC execs before returning the stream.

I’ve patched the actual protocol mismatch now: fresh v2 SSH execs discard the capability advertisement before exposing the command stream. I’m rerunning both the focused SSH unit tests and the end-to-end shim integration test to confirm that fix against the regression it just exposed.

The v2 trim logic was right, but I had a bad error-path variable reuse that only showed up in the repeated-RPC unit test. That’s fixed; I’m rerunning the SSH unit and integration checks together again.

The integration test found another SSH-specific behavior difference: git-receive-pack can emit a valid advertised-refs response and still exit non-zero once the client disconnects after discovery. I’m treating “data was read successfully” as success for RequestInfoRefs, which matches how this discovery step is actually used.

I’ve adjusted discovery to accept a complete advertised-refs payload even if the remote exits non-zero after the client closes the session. That should unblock target ref discovery over SSH without weakening the actual fetch/push paths.

The push failure came from the same underlying rule, just on the target side: every fresh SSH exec starts with an advertisement section, including receive-pack. I’m generalizing the transport so every SSH RPC trims an initial pkt-line advertisement when present, and leaves non-pkt-line streams alone.

I’ve generalized the SSH stream handling now: trim the leading advertisement section when the stream starts with pkt-line framing, otherwise leave it untouched. That should cover upload-pack v1/v2 and receive-pack while keeping the unit shims simple.

The end-to-end SSH shim test is passing now. I’m adding a small docs commit next so the branch explains the supported URL forms and the current --progress / --show-stats caveat instead of leaving that buried in code and tests.

I’m keeping the docs update tight: usage docs for the supported SSH URL forms and current caveat, README FAQ so it stops saying “no,” and a small protocol-doc note that the deep wire walkthrough is still HTTP-focused.

The implementation is functionally complete at this point. I’m running the final SSH-focused test slice across gitproto and syncer, then I’ll cut the remaining commits as integration coverage and docs.

The branch is now in the state we planned: four reviewable SSH commits on top of main, with the integration test and docs split cleanly. I’m doing a final sanity check on the branch status and commit list, then I’ll summarize exactly what landed and the remaining caveat.

Implemented on branch soph/ssh-transport.

Commits:

• f44c0ccc gitproto: extract transport interface and rename HTTPConn
• ebfc0d98 gitproto: add SSH transport via per-RPC ssh exec
• 74a4cf49 syncer: warn that SSH stats and progress are incomplete
• c0b12714 tests: add end-to-end SSH transport coverage
• 98ca4e96 docs: document SSH support and caveats

What landed:

• SSH transport via the local ssh binary, supporting ssh://, SCP-style git@host:path.git, and git+ssh://.
• Per-RPC SSH execution, so repeated source/target RPCs work across protocol v2, bootstrap, and push flows.
• SSH handling for the extra advertisement phase that upload-pack / receive-pack emit on fresh execs.
• Syncer transport selection for SSH endpoints.
• A warning path for --progress / --show-stats, since SSH byte-counted throughput is still incomplete.
• Docs updated in README.md, docs/usage.md, and docs/protocol.md.

Tests I ran:

• go test ./internal/gitproto ./internal/syncer -run 'TestSSHConn|TestRun_IntegrationSyncOverSSHShimV2|TestSSHStatsWarning|TestNewConnBuildsSSHTransport|TestGitHubOwnerRepo|TestNewHTTPConn_PropagatesFollowInfoRefsRedirect|TestNewHTTPConnSkipTLSVerify|TestNewHTTPConnUsesProvidedHTTPClient' -count=1
• go test ./internal/gitproto ./internal/strategy/... ./internal/syncer -run '^$'
• Focused transport-refactor tests with local listener permission earlier.

Remaining caveat:

• SSH transport works, but --progress and --show-stats still do not include byte-counted SSH throughput. The branch warns about that explicitly instead of silently showing misleading zeros.

The current shim test is a real integration test, but not yet the strongest possible e2e test.

A proper ladder is:

1. Keep the current shim test It already proves the important transport behavior:

• git-sync chooses SSH based on URL shape
• it shells out to ssh
• repeated RPCs work
• protocol v2 source discovery works
• target push works against real git-upload-pack / git-receive-pack

That is a good CI-stable integration test because it has no daemon dependency.

2. Add one “real sshd” e2e test behind a gate For true end-to-end coverage, run:

• a temporary local sshd
• a temp home dir with known_hosts, client key, and minimal ssh_config
• one or two bare repos exposed through that server
• git-sync against ssh://127.0.0.1:<port>/...

That catches the things the shim cannot:

• real key auth
• host key checking
• actual sshd command dispatch behavior
• quoting / environment differences
• port handling through the real client/server path

I would gate it behind something like:

• build tag e2e_ssh, or
• env var GITSYNC_TEST_SSHD=1

So CI can run the shim test always, and the heavier sshd test only in the right environment.

3. What the sshd e2e should verify At minimum:

• source over SSH, target over SSH
• --protocol=auto negotiates v2
• one flow that requires multiple source RPCs
• one flow that requires target discovery plus push
• SCP-style URL and ssh:// URL
• non-default port
• user from SSH config, not forced git@
• failure when host key is unknown in batch mode
• success when host key is preloaded
• cancellation timeout surfaces context failure cleanly

4. Practical implementation options Best options:

• Start sshd if present on the machine, skip otherwise.
• Or use a containerized sshd fixture if your CI allows Docker.
• Or vendor a tiny test harness script that writes temp config and launches sshd -D -f <config>.

I would not try to fake more of SSH in-process. At that point you’re rebuilding the thing you want to validate.

5. Recommended repo strategy Use both:

• internal/syncer/ssh_integration_test.go: fast shim-based integration, always on
• internal/syncer/ssh_e2e_test.go: real sshd e2e, gated

That gives you reliable coverage in normal CI and one high-confidence path for release validation.

If you want, I can sketch the exact sshd fixture layout and test harness next.

Yes, Docker is a reasonable strategy for the SSH e2e path. I would use it selectively, not as the default testing substrate for everything.

For SSH:

• Docker is a good fit because sshd is the hard part.
• A container gives you a real server, fixed host keys, predictable config, and easy non-default port coverage.
• That makes it much better than trying to bootstrap host sshd across developer machines and CI images.

For HTTP:

• I would not move the existing HTTP integration suite to Docker by default.
• Your current HTTP tests are already strong and much cheaper: in-process httptest and git http-backend give real protocol coverage without container startup, port plumbing, or CI flakiness.
• Docker would add runtime and operational complexity without buying much unless you specifically want to validate against a real external server stack like Apache/nginx+cgi.

So the split I’d recommend is:

• HTTP:

  • Keep the current in-process / git http-backend tests as the main path.
  • Only add Docker if you have a specific bug class that depends on a real HTTP deployment environment.

• SSH:

  • Keep the current shim integration test as the always-on test.
  • Add a Docker-backed sshd e2e test as an opt-in or nightly/CI-expanded test.

Why this split is good:

• Fast tests stay fast.
• SSH gets one true end-to-end environment.
• You avoid over-containerizing paths that are already well covered.

A practical structure would be:

• internal/syncer/ssh_integration_test.go
  • always on, shim-based
• internal/syncer/ssh_e2e_test.go
  • Docker-backed, gated by env var or build tag
• Optional later:
  • internal/syncer/http_e2e_test.go
  • only if you find a real need to validate against a containerized HTTP server stack

If you use Docker for SSH, I’d make the test harness:

• build or pull a tiny sshd image
• mount temp bare repos
• mount authorized keys + host keys + config
• expose one port
• run git-sync against ssh://127.0.0.1:<port>/...
• preload known_hosts in a temp home or pass a temp ssh config via environment if you later add that hook

The main caution is CI dependency:

• If Docker is not guaranteed, gate the test.
• Do not make core PR validation depend exclusively on Docker-based e2e.

So: Docker for SSH, yes. Docker for HTTP, probably not worth it right now.

Not in this PR.

You already have:

• fast always-on SSH integration via the shim
• real transport behavior covered in unit tests
• end-to-end sync coverage over SSH without daemon dependencies

That is enough to merge the feature.

I would add Docker-backed SSH e2e next if:

• you want release-confidence coverage for host keys / real auth / real sshd
• your CI can run Docker reliably
• you’re willing to maintain one more test harness

Why not now:

• it expands scope from “ship SSH support” to “ship SSH support plus containerized test infrastructure”
• the feature risk is already reduced a lot by the shim test
• Docker test harnesses tend to add the most churn around CI, timing, cleanup, and portability

Best next step:

• merge this branch first
• open a follow-up PR for ssh_e2e_test.go
• gate it behind GITSYNC_TEST_SSHD=1 or a build tag
• optionally run it only in a dedicated CI job

If you want, I can prepare that follow-up PR next, but I would keep it separate from the transport feature PR.

I’m fixing the two merge-blocking issues now: the wrong user-facing flag name and SCP-style ~/repo.git handling in SSH URLs. I’ll also add the missing stream-cancellation test while I’m in the SSH transport.

I’m folding in the review fixes beyond the blockers while I’m touching the files: --stats wording, ~-prefixed SCP paths, a stream-cancellation test, and a short doc note about missing ssh on PATH.

I’ve applied the review fixes. I’m running the SSH-focused tests now, especially the new ~-path and stream-cancellation cases, to make sure the transport behavior still matches the integration coverage.

I accidentally included a Markdown file in gofmt; the code changes themselves are unaffected. I’m rerunning formatting on just the Go files and then rechecking the SSH test slice.

The new cancellation test exposed the actual contract: the stream read can reach EOF cleanly, and the context failure surfaces on Close(), which is where the transport waits for the remote process. I’m adjusting the test to assert that behavior instead of forcing ReadAll to fail.

The review fixes are green. I’m checking the working tree and then I’ll tell you exactly what changed so you can decide whether to fold this into the last docs/test commits or keep it as a small follow-up commit on the branch.

I’m packaging the review fixes as one small follow-up commit: user-facing flag text, ~-path handling, and the extra SSH stream test. That keeps the original commit series intact and makes the review delta easy to inspect.

Fixed the review items in follow-up commit ceeddf92 on soph/ssh-transport.

What changed:

• User-facing warning/docs now correctly reference --stats, not --show-stats.
• SSH remote command quoting now preserves ~-prefixed SCP-style paths like git@example.com:~/repo.git, instead of single-quoting away shell expansion.
• Added a PostRPCStreamBody cancellation test to cover the stream path, not just RequestInfoRefs.
• Added a short doc note that missing ssh on PATH fails early with locate ssh binary.
• Added a clarifying comment for the unused phase parameter in the SSH transport.

Verification:

• go test ./internal/gitproto ./internal/syncer -run 'TestSSHConn|TestRun_IntegrationSyncOverSSHShimV2|TestSSHStatsWarning' -count=1

Branch head is now:

• ceeddf92 ssh: fix review follow-ups

Agreed. I’d merge this branch as-is.

The only extra thing I’d consider before opening the PR is that one comment near the direct warning write in newSession, just to make the pre-progress timing explicit. That’s optional and not worth another logic change.

Current branch tip:

• ceeddf92 ssh: fix review follow-ups

Full commit stack:

• f44c0ccc gitproto: extract transport interface and rename HTTPConn
• ebfc0d98 gitproto: add SSH transport via per-RPC ssh exec
• 74a4cf49 syncer: warn that SSH stats and progress are incomplete
• c0b12714 tests: add end-to-end SSH transport coverage
• 98ca4e96 docs: document SSH support and caveats
• ceeddf92 ssh: fix review follow-ups

If you want, I can add that one comment and open the PR description text next.

I’m checking the branch state and upstream first, then I’ll push soph/ssh-transport if needed and open the PR with a summary of the commit stack, behavior, tests, and current caveat.

The branch is clean and has no upstream yet. I need to push soph/ssh-transport to origin before I can open the PR.

The push is running through the repo’s normal git hook flow right now. I’m waiting for it to finish before creating the PR.

The branch is pushed. I’m creating the PR now with a focused summary of the transport refactor, SSH behavior, tests, and the remaining stats/progress caveat.

The first gh pr create call only failed because I hadn’t provided the non-interactive title/body yet. I’m submitting it now with the full description.

The shell ate the backticks in the PR body, so that attempt was just a quoting failure, not a repo issue. I’m writing the body to a temp file and creating the PR from that to avoid shell interpolation entirely.

The PR content is ready; the only failure now is network access to GitHub from the sandbox. I’m rerunning the gh pr create step with escalation so it can reach the GitHub API.

PR created: https://github.com/entireio/git-sync/pull/54

Title:

• Add SSH transport support

Branch:

• soph/ssh-transport

It includes the full commit stack plus the follow-up review fixes.

I’m running the project lint task to see the exact failures, then I’ll fix only the issues it reports and rerun it cleanly.

mise run lint is failing because the sandbox cannot reach proxy.golang.org and golangci-lint.run, so the lint task can’t download modules or the JSON schema. I need to rerun the lint task with network access to get the real lint result and then fix any actual issues it reports.

The lint task is now running with network access and downloading dependencies. I’m waiting for the real lint verdict before changing anything.

The remaining mise run lint failure is not from the SSH code. golangci-lint is typechecking before the isolated module cache is primed, and the gomod task is also trying to resolve latest module state in /tmp, which drifts to go-git alpha.3 and breaks package lookups. I’m checking the lint task wiring so I can fix the repo-local lint path rather than papering over it per shell session.

I’m checking whether this is an artifact of the isolated temp module cache I used or a real repo issue with the lint scripts. If plain mise run lint works once the normal cache is available, I’ll avoid changing repo tooling unnecessarily.

This is the real signal. The normal lint path is using the existing module cache cleanly, so I’m waiting for the actual golangci-lint output rather than the temp-cache artifact from earlier.

I have the actual lint findings now. They’re all local code-style issues from the transport refactor and SSH transport, not module/tooling problems, and I’m fixing them directly.

I’ve fixed the linter findings in-place: wrapped interface/external errors, normalized HTTPConn receiver naming, handled the ignored cmd.wait result, and added a targeted ireturn suppression where the interface return is intentional. I’m rerunning mise run lint now to confirm it’s clean end-to-end.

mise run lint is clean now.

What I changed:

• wrapped delegated/interface errors in internal/gitproto/smarthttp.go
• normalized HTTPConn receiver names there
• handled the ignored cmd.wait() error and wrapped a few external errors in internal/gitproto/ssh.go
• added the intentional ireturn suppression and wrapped NewSSHConn errors in internal/syncer/syncer.go

Verification:

• mise run lint passes end-to-end

If you want, I can commit these lint fixes and update the PR branch.

I’m committing just the lint-fix delta and then I’ll give you the new branch tip so the PR is current.

Committed as 3cc55f83 lint: fix transport linter findings.

The PR branch now includes the lint-cleanup commit on top. If you want, I can push the updated branch to PR #54 as well.

I’m fixing the leaked-process cleanup in SSHConn.RequestInfoRefs and adding a regression test that forces stdin.Close() to fail so the early-return path is exercised.

I’m fixing this in the transport itself and adding a regression test around the exact early-return path, using a fake sshCommand so the test can force stdin.Close() to fail deterministically.

The cleanup logic and regression test are in. I’m formatting and running the focused SSH transport tests now to verify the leak fix without disturbing the rest of the branch.

Fixed.

What changed:

• SSHConn.RequestInfoRefs now cleans up properly if cmd.Stdin.Close() fails:
  • closes cmd.Stdout
  • waits on the child process
  • joins cleanup errors into the returned error
• added a regression test that forces stdin.Close() to fail and asserts both stdout.Close() and wait() are called

Files:

• internal/gitproto/ssh.go
• internal/gitproto/ssh_test.go

Verification:

• go test ./internal/gitproto -run 'TestSSHConn|TestRequestInfoRefsCleansUpWhenStdinCloseFails' -count=1

If you want, I can commit this fix onto the PR branch too.

I’m committing the SSH cleanup fix now so the PR branch includes the leak regression test and transport cleanup change.

Committed as 9c50a33f ssh: clean up failed info-refs startup.

If you want, I can push the latest branch tip to update PR #54 now.