Conversation
Entire-Checkpoint: 2dcf2f1a3166
Entire-Checkpoint: 0bdb781a9478
Entire-Checkpoint: f09ed59d22eb
Entire-Checkpoint: 68e3d42cfb81
Entire-Checkpoint: 72f07f367d81
Entire-Checkpoint: 4805c08b89a8
Entire-Checkpoint: 8620d1269809
Entire-Checkpoint: 4a7575f2583f
Entire-Checkpoint: 894a895034bb
Entire-Checkpoint: 03bc9b185ab8
Entire-Checkpoint: 78bcff081d69
Entire-Checkpoint: 90fce803a9ea
Entire-Checkpoint: 896f021c4767
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 5 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit d971ec0. Configure here.
| Protocol: sourceService.protocol, | ||
| }, nil | ||
| } | ||
| return bootstrapWithInputs(ctx, cfg, stats, sourceConn, targetConn, sourceService, targetAdv, desiredRefs, targetRefMap, reason) |
There was a problem hiding this comment.
Auto-bootstrap path missing measurement, leaking goroutine
High Severity
When the Run function takes the non-dry-run bootstrap relay path, it returns early via bootstrapWithInputs without calling the measurementDone() closure. This prevents the memory measurement goroutine from stopping, causing a leak, and results in a zero-valued Measurement in the returned Result.
Reviewed by Cursor Bugbot for commit d971ec0. Configure here.
| RelayMode: "", | ||
| RelayReason: "", | ||
| Stats: stats.snapshot(), | ||
| Measurement: measurementDone(), |
There was a problem hiding this comment.
Measurement frozen before push operations complete
Medium Severity
The Run function's memory measurement is finalized prematurely. measurementDone() is called when initializing the Result struct, before any push or incremental relay operations. Since startMeasurement uses sync.Once, this early invocation freezes the measurement, causing it to miss the elapsed time, peak memory, and GC activity from the actual push phase.
Reviewed by Cursor Bugbot for commit d971ec0. Configure here.
|
|
||
| There is a planned `bootstrap` command path for large initial syncs into an empty target. The intent is to relay a fetched source pack directly into target `receive-pack` instead of decoding the full object graph into local memory first. | ||
|
|
||
| The design note is in [docs/bootstrap.md](/Users/soph/Work/entire/devenv/git-sync/docs/bootstrap.md). |
There was a problem hiding this comment.
Local filesystem path accidentally committed in README
Medium Severity
The README.md includes a link to docs/bootstrap.md that uses an absolute local filesystem path. This makes the link broken for other users and exposes a local machine path.
Reviewed by Cursor Bugbot for commit d971ec0. Configure here.
| } else { | ||
| return reason | ||
| } | ||
| } |
There was a problem hiding this comment.
Redundant if/else always returns same value
Low Severity
The relayFallbackReason function's if/else statement always returns the same reason value. This makes the ok check from canIncrementalRelay redundant and suggests the conditional was intended for distinct logic.
Reviewed by Cursor Bugbot for commit d971ec0. Configure here.
| return result, fmt.Errorf("fetch source pack: %w", err) | ||
| } | ||
| defer packReader.Close() | ||
| packReader = limitPackReadCloser(packReader, cfg.MaxPackBytes) |
There was a problem hiding this comment.
Pack reader double-closed after successful push
Medium Severity
In both bootstrapWithInputs and the incremental relay path in Run(), defer packReader.Close() is registered, and then packReader is reassigned to a limitPackReadCloser wrapper. On success, receivePackStream calls pack.Close() on the wrapper. Then the deferred close fires on the same wrapper (Go defers capture the variable, not the value), causing a double close of the underlying reader/session. For the V1 path's sessionReadCloser, this can double-close the upload-pack session.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d971ec0. Configure here.
Entire-Checkpoint: 9425efd8edfc
Entire-Checkpoint: d005afe6724c
Entire-Checkpoint: d50053e4c868
Entire-Checkpoint: a2d74df554b4
Entire-Checkpoint: aebdecbeb417
Entire-Checkpoint: 5cc48c743bfd
Entire-Checkpoint: a859cf299e7d
Entire-Checkpoint: 4adfd6c1e723
Entire-Checkpoint: ffd71d2ccb2a
Entire-Checkpoint: 4733a258b7e6
Entire-Checkpoint: 6ef2787ebf99
Entire-Checkpoint: 21576324299f
Entire-Checkpoint: 6afc8032e5a1
Entire-Checkpoint: 0670ea271363
Entire-Checkpoint: 121669700ffc
Entire-Checkpoint: 2fd4174c6988
Entire-Checkpoint: 67706ea6e332
Entire-Checkpoint: 3ad59c31a883
Entire-Checkpoint: 33dc63f83cff
Break the monolithic syncer.go (3143 lines) into 7 focused packages: - internal/gitproto: pkt-line, smart HTTP, capability negotiation, v1/v2 fetch/push - internal/planner: mapping validation, planning, relay eligibility, checkpoints - internal/auth: credential resolution, Entire DB tokens, git credential helper - internal/strategy/bootstrap: one-shot + batched bootstrap, GitHub preflight - internal/strategy/incremental: incremental relay execution - internal/strategy/materialized: materialized fallback push with size guard - internal/syncer: slim orchestrator (734 lines), stats, measurement Addresses all 22 issues from docs/rewrite-issue-list.md: Correctness: tag ref creation independent of pack (#1), duplicate target mapping rejection (#2), cross-kind mapping rejection (#3), sideband-64k preference (#4), pack reader close discipline (#5), include-tag capability gating (#6), OAuth refresh error propagation (#7). Concurrency: mutex-protected stats (#8), bounded response reads (#9), flock-based file token store locking (#10). Architecture: package decomposition (#11), shared session setup (#12), explicit Params structs (#13). Performance: commit-count batch sizing heuristic (#14), materialized object count guard (#15), bounded ancestry checks with ErrAncestryDepthExceeded (#16), reusable pkt-line buffer (#17). Testing: 73 test functions, 7 benchmarks, coverage 41-58% on core packages. Protocol malformed-input tests (#18-20), behavioral edge cases (#21), benchmarks for planning/protocol hot paths (#22). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 27633b8ca595
Closes the credential-leak advisory tracked as CVE-2026-41506 (GHSA-3xc5-wrhm-f963 / Dependabot alert #1). Our smart-HTTP path already used Go's stdlib http.Client directly, which strips the Authorization header on cross-host redirects since 1.8 — but upgrading clears the alert and pulls in the upstream http.followRedirects controls. Alpha.2 is a major rewrite of plumbing/transport. Translation: - *transport.Endpoint (struct) → *url.URL throughout. Field accesses (.Scheme, .Host, .Path, .User, .Hostname()) are unchanged. - transport.NewEndpoint → transport.ParseURL. - transport.AuthMethod (interface) is gone. Defined our own auth.Method and gitproto.AuthMethod with a single Authorizer(*http.Request) error method, satisfied by *transporthttp.BasicAuth and *transporthttp.TokenAuth (whose SetAuth methods were renamed to Authorizer). - transport.Service (typed) → string constants. Function parameters take string. - transporthttp.NewTransport(*TransportOptions) → NewTransport(Options) (value, not pointer). - transport.AdvertiseReferences → transport.AdvertiseRefs. - transport.UploadPackOptions → transport.UploadPackRequest; transport.ReceivePackOptions → transport.ReceivePackRequest. - transport.Register / transport.Get were removed. The TestMain shims in syncer/integration_test.go and cmd/git-sync/main_test.go registered a custom HTTP transport for go-git's transport registry, but our code never goes through that registry — it hits the network through gitproto's own http.Client. Dropped both shims as dead code. Also dropped the now-unused Conn.Transport field; nothing in git-sync read it. Updated .golangci.yaml ireturn allowlist to permit the new auth.Method interface where the previous transport.AuthMethod allowance lived. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: b0d39f2320f3
Closes the credential-leak advisory tracked as CVE-2026-41506 (GHSA-3xc5-wrhm-f963 / Dependabot alert #1). Our smart-HTTP path already used Go's stdlib http.Client directly, which strips the Authorization header on cross-host redirects since 1.8 — but upgrading clears the alert and pulls in the upstream http.followRedirects controls. Alpha.2 is a major rewrite of plumbing/transport. Translation: - *transport.Endpoint (struct) → *url.URL throughout. Field accesses (.Scheme, .Host, .Path, .User, .Hostname()) are unchanged. - transport.NewEndpoint → transport.ParseURL. - transport.AuthMethod (interface) is gone. Defined our own auth.Method and gitproto.AuthMethod with a single Authorizer(*http.Request) error method, satisfied by *transporthttp.BasicAuth and *transporthttp.TokenAuth (whose SetAuth methods were renamed to Authorizer). - transport.Service (typed) → string constants. Function parameters take string. - transporthttp.NewTransport(*TransportOptions) → NewTransport(Options) (value, not pointer). - transport.AdvertiseReferences → transport.AdvertiseRefs. - transport.UploadPackOptions → transport.UploadPackRequest; transport.ReceivePackOptions → transport.ReceivePackRequest. - transport.Register / transport.Get were removed. The TestMain shims in syncer/integration_test.go and cmd/git-sync/main_test.go registered a custom HTTP transport for go-git's transport registry, but our code never goes through that registry — it hits the network through gitproto's own http.Client. Dropped both shims as dead code. Also dropped the now-unused Conn.Transport field; nothing in git-sync read it. Updated .golangci.yaml ireturn allowlist to permit the new auth.Method interface where the previous transport.AuthMethod allowance lived. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: b0d39f2320f3
1 participant
Summary
This branch adds a new relay-based sync path for initial and selected incremental remote-to-remote Git syncs, plus the harness and observability needed to validate it.
The main changes are:
bootstrapcommand for create-only initial seeding into empty targetssyncwhen the managed target refs are absentsyncfor relay-safe non-empty target updatesgit-http-backendWhat Changed
bootstrapgit-sync bootstrap <source> <target>as a separate create-only relay path.receive-packinstead of decoding objects into the local object store first.--map--tags--stats--json--protocol auto|v1|v2--max-pack-bytes--force,--prune, or dry-run.Automatic empty-target relay
syncnow auto-selects the bootstrap relay path when all managed target refs are absent and the run matches bootstrap semantics.plandoes not execute relay, but it now surfaces when bootstrap is the appropriate path.Incremental relay
syncfor relay-safe non-empty target updates.--force--pruneMeasurement and diagnostics
--measure-memorytosync,plan,bootstrap,probe, andfetch.elapsed_millispeak_alloc_bytespeak_heap_inuse_bytestotal_alloc_bytesgc_countplan/syncoutput now also includes:relayrelay_moderelay_reasonAuth and testability
git credential fillwhen explicit flags/env vars are not provided.Integration coverage
git-http-backendend-to-end tests for:torvalds/linuxfor large-source fetch/measurement checks.Why
The original sync path works well for general incremental reconciliation, but it is a poor fit for large initial syncs because it materializes fetched objects locally before pushing them onward.
This branch introduces relay-based paths to reduce local memory and CPU cost in the cases where that is safest:
It also adds the measurement and test harness needed to validate where relay makes sense and where normal decode/repack behavior should remain the default.
Safety / Scope
Relay is intentionally conservative.
sync.How To Try It
Initial seeding:
Normal sync with automatic empty-target relay:
Compare memory behavior:
Optional local backend tests:
Validation
env GOCACHE=/tmp/go-build go test ./...git-http-backendtests passed for bootstrap, incremental relay expansions, and diverged-target blockingFollow-Up
Not included in this branch:
Those are possible next steps, but they are meaningfully riskier than the relay-safe scope added here.
Note
Medium Risk
Adds new pack-relay execution paths (including a new
bootstrapcommand and automatic relay selection insync) that change how objects are fetched/pushed and could surface edge-case protocol/capability issues across remotes. Risk is mitigated by conservative gating (no force/prune, fast-forward/create-only constraints) and expanded integration/e2e coverage.Overview
Adds a new relay-based sync mode to avoid local decode/repack when it’s safe. Introduces
git-sync bootstrapfor create-only seeding of empty targets by streaming the source pack directly into targetreceive-pack, including a--max-pack-bytessafety limit.Enhances
sync/planbehavior and diagnostics:syncauto-selects the bootstrap relay path when all managed target refs are absent, and adds a narrow incremental relay path for relay-safe fast-forward branch updates and tag creation (with fallback to the existing path otherwise). JSON/text output now includesrelay,relay_mode,relay_reason, andbootstrap_suggestedto explain decisions.Adds observability and tests: introduces
--measure-memoryacross commands withmeasurementfields in outputs, adds a live read-only Linux fetch smoke test, and significantly expands integration andgit-http-backendend-to-end tests to cover bootstrap, incremental relay scenarios, and diverged-target blocking.Reviewed by Cursor Bugbot for commit d971ec0. Configure here.