Review commit b718e82 ("Add batched planning backend sensitivity test") in /Users/soph/Work/entire/devenv/git-sync. Focus on whether the new git-http-backend test is a sound experiment surface for bat · Entire

Home

Log in

can you get yourself an overview over the repo, and then the implemented code, and then kick of specific agents reviewing everything. One of them should look at "what if we would write this from scratch, what would we do differently"

3mo ago·27 calls

I’m getting a top-level view of the repository first, then I’ll read the main implementation paths so the review delegation is grounded in the actual code rather than file names. After that I’ll spin up focused agents, including one tasked with a from-scratch redesign critique.

The repo is compact: a single CLI entrypoint and one substantive package under internal/syncer, with tests doing a lot of the behavioral specification. I’m reading the core docs and implementation now so I can split review ownership along real architectural seams.

I’ve confirmed the shape: the bulk of the system lives in one large syncer.go file plus the protocol-v2 helper, with docs and tests describing the intended relay/batching behavior. Next I’m mapping the main execution paths and the test surface, then I’ll delegate review agents against those slices.

I have enough context to split the review cleanly. The codebase is effectively four concerns: CLI surface in cmd/git-sync, the main orchestration/planning/push path in internal/syncer/syncer.go, protocol-v2 and wire helpers in internal/syncer/protocol_v2.go, and a fairly strong integration-heavy test suite.

Review split I’m kicking off:

  1. Core execution and correctness: planning, relay selection, bootstrap, batching, push/fallback behavior.
  2. Protocol and transport internals: pkt-line handling, v1/v2 negotiation, stream safety, HTTP/auth behavior.
  3. Test strategy and gaps: what behavior is actually locked down, and what regressions still look plausible.
  4. From-scratch redesign: if we rebuilt this today, what architecture and interfaces would we choose differently.

The review agents are running now. While they work, I’m doing my own pass on the highest-risk paths so I can synthesize their findings instead of just relaying them.

The main implementation pass confirms the current architecture is deliberately optimized around streamed relay paths and fallback logic, but most of that behavior is still concentrated in a single orchestration file. I’m waiting on the delegated reviewers now and will merge their findings into one high-signal summary.

<subagent_notification> {"agent_path":"019d7d09-b4a2-74f0-b87f-791514eb33b8","status":{"completed":"Findings\n\n1. High: destructive ref-selection and prune semantics are only lightly specified by tests, despite living in the most dangerous code paths. buildDesiredRefs and buildPlans decide what is managed, mirrored, or deleted across branches, mappings, tags, and --prune ( internal/syncer/syncer.go:1651, internal/syncer/syncer.go:1750). The suite covers one broad prune case for tags plus force divergence ( internal/syncer/integration_test.go:696), but I did not find focused tests for: mixed --branch selection with --prune, mapping collisions, duplicate mappings, pruning with explicit mappings, or exact-ref mappings involving tags. A regression here is not “just wrong output”; it can delete or retarget remote refs.\n\n2. High: bootstrap batching has real implementation complexity but mostly happy-path coverage. The critical planning/execution path spans checkpoint planning, sampling heuristics, resume behavior, temp-ref cutover, and tag-after-branches ordering ( internal/syncer/syncer.go:1144, internal/syncer/syncer.go:1398, internal/syncer/syncer.go:1475). Tests do cover end-to-end batching, resume, and tags with git-http-backend ( internal/syncer/git_http_backend_test.go:406, internal/syncer/git_http_backend_test.go:471, internal/syncer/git_http_backend_test.go:563), but the heuristic itself only has tiny helper-level tests for candidate sampling and probe selection ( internal/syncer/syncer_test.go:118, internal/syncer/syncer_test.go:134). Missing are failure-path tests for mismatched temp refs, “no checkpoint fits”, multi-branch batching interactions, and partial cutover failures. That is a lot of untested state-transition logic.\n\n3. Medium: protocol-v2 parsing and wire-format error handling are under-specified by tests. The parser and request builders are hand-rolled ( internal/syncer/protocol_v2.go:75, internal/syncer/protocol_v2.go:132, internal/syncer/protocol_v2.go:179). Tests cover special packets, one capability advertisement, one request encoding, and include-tag insertion ( internal/syncer/protocol_v2_test.go:10, internal/syncer/protocol_v2_test.go:46, internal/syncer/protocol_v2_test.go:69, internal/syncer/protocol_v2_test.go:91). I did not find tests for malformed pkt-lines, unexpected delimiters/response-end placement, truncated payloads, unsupported v2 capability ads, or stream decoding failures. Given the amount of custom wire handling, this is a meaningful gap.\n\n4. Medium: the CLI surface is barely tested relative to its branching behavior. cmd/git-sync/main.go owns flag parsing, env defaults, positional-vs-flag URL resolution, and command dispatch ( cmd/git-sync/main.go:23, cmd/git-sync/main.go:46, cmd/git-sync/main.go:120). Tests only check JSON marshaling and that plan --json does not push ( cmd/git-sync/main_test.go:34, cmd/git-sync/main_test.go:87). There is no direct coverage for invalid mappings, unsupported protocol values, bootstrap rejecting unsupported flags, positional argument precedence, or usage/help behavior. For a CLI tool, that is thin.\n\n5. Medium: auth coverage is useful but narrow. The tests hit explicit token precedence, SkipTLSVerify, stored Entire token lookup, and refresh flow ( internal/syncer/auth_test.go:16, internal/syncer/auth_test.go:48, internal/syncer/auth_test.go:70, internal/syncer/auth_test.go:110), plus one integration test for git credential helper fallback ( internal/syncer/integration_test.go:482). Missing are bearer-token precedence, malformed helper output, refresh failures, keyring/file-store fallback permutations, and source-vs-target auth asymmetry. The risk is mostly operational rather than algorithmic, but it is still a surface area with a lot of conditionals.\n\nWhat the current tests give confidence in\n\n- The core product path is meaningfully exercised end to end: empty-target bootstrap relay, normal incremental sync, protocol-v2 fetch/probe/fetch-haves, branch mapping, dry-run behavior, tag creation, force/prune, and stats all have integration coverage ( internal/syncer/integration_test.go:36, internal/syncer/integration_test.go:338, internal/syncer/integration_test.go:404, internal/syncer/integration_test.go:530, internal/syncer/integration_test.go:696).\n\n- Compatibility with a real Git server stack is better than average for a small repo. The git-http-backend tests validate bootstrap, incremental relay, multi-branch fast-forward, mapped-branch fast-forward, tag create relay, batched bootstrap, resume, and batched tags against actual Git CGI behavior ( internal/syncer/git_http_backend_test.go:23, internal/syncer/git_http_backend_test.go:155, internal/syncer/git_http_backend_test.go:227, internal/syncer/git_http_backend_test.go:298, internal/syncer/git_http_backend_test.go:406).\n\n- The suite is strongest where it checks externally visible behavior and weakest where it relies on complex internal heuristics and policy decisions. That means I’d trust the happy-path transport behavior more than I’d trust edge-case ref management or batching planner correctness."}} </subagent_notification>

3mo ago·1 call

One review has landed and it lines up with the shape of the code: strong end-to-end confidence on happy paths, much weaker specification around destructive ref policy and batching edge cases. I’m waiting for the remaining reviewers so I can give you one consolidated read rather than four disconnected reports.

<subagent_notification> {"agent_path":"019d7d09-b4c5-7fa2-b5dc-e35b0979d316","status":{"completed":"Repo overview is in place. This is a small Go CLI with one thin entrypoint in cmd/git-sync/main.go and almost all implementation in internal/syncer/syncer.go plus protocol-v2 wire handling in internal/syncer/protocol_v2.go. The product shape is clear: remote-to-remote smart-HTTP mirroring, with normal sync, empty-target bootstrap relay, incremental relay, and a batched bootstrap fallback for very large repos. Docs in README.md, docs/bootstrap.md, and docs/bootstrap-batching.md match the implemented code reasonably well.\n\nImplemented-code overview: main.go is mostly flag plumbing and output formatting; the real behavior sits in syncer.Run, syncer.Bootstrap, syncer.Probe, and syncer.Fetch. The biggest implementation concentration is that syncer.go mixes planning, relay selection, batching, push mechanics, transport/auth, stats, and credential refresh in one file, which is the main architectural pressure point. The test surface is stronger than the repo size suggests: unit tests, in-process smart-HTTP integration, optional git-http-backend tests, and some live/smoke paths. go test ./... passed locally.\n\nI kicked off four focused review agents:\n1. Core execution/correctness: planning, bootstrap, incremental relay, batching, push/fallback, CLI-to-core config.\n2. Protocol/transport internals: pkt-line parsing, v1/v2 negotiation, stream handling, HTTP/auth/error paths.\n3. Test strategy/gaps: what behavior is actually locked down and where regressions could still hide.\n4. From-scratch redesign: what we would keep, what we would redesign, and how to migrate there.\n\nMy own immediate read is that the core idea is sound, and the main risk is not “wrong product direction” but “too many concerns collapsed into one package,” especially around batching and auth/transport interactions. If you want, next I can turn this into a consolidated review document with prioritized findings and the from-scratch redesign critique."}} </subagent_notification>

3mo ago·0.0s·2 calls

I’ve got the structural picture and two of the review threads back. I’m running the local test suite once myself so the final summary includes the current verification state, not just static review.

<subagent_notification> {"agent_path":"019d7d09-b47c-70f1-abb2-e81b78135584","status":{"completed":" Repo Overview\ngit-sync is a small Go repo with one CLI entrypoint in cmd/git-sync/main.go and almost all implementation in internal/syncer/syncer.go plus internal/syncer/protocol_v2.go. The product shape is clear: discover source/target refs over smart HTTP, build a ref plan, then choose one of three execution strategies: bootstrap relay, incremental relay, or local materialize-and-push. The tests are substantial and mostly integration-driven, which is appropriate for this kind of protocol-heavy tool.\n\nI couldn’t literally fan out to child review agents because that orchestration tool is not exposed in this session, so I split the work into agent-style review tracks and ran them directly. I also ran go test ./...; the suite passes.\n\n Findings\n1. Protocol-v2 tag fetches can request include-tag without checking whether the server advertised support. In internal/syncer/protocol_v2.go, sourceFetchRequestV2 unconditionally appends include-tag when any desired ref is a tag ( lines 654-658). The v1 path correctly gates include-tag on capability support, but the v2 path does not. On stricter servers this can turn a valid tag sync into a protocol error.\n\n2. Mapping collisions are silently accepted and last-write-wins. In internal/syncer/syncer.go, buildDesiredRefs writes directly into desired[targetRef] and managed[targetRef] with no duplicate detection ( lines 1655-1668). For a mirroring tool, allowing --map main:stable --map release:stable to silently collapse into one target ref is risky and hard to diagnose.\n\n3. Expired EntireDB tokens can be reused silently after refresh failure. In internal/syncer/syncer.go, getEntireDBTokenWithRefresh falls back to returning the stale token when refresh fails ( lines 2787-2790). That turns a token-refresh problem into a later auth failure against the Git remote, which obscures the real cause and can create flaky behavior around expiry boundaries.\n\n4. Core execution logic is carrying too much unrelated responsibility in one file. internal/syncer/syncer.go mixes planning, strategy selection, bootstrap batching, pack push mechanics, auth resolution, token refresh, stats, and measurement. That is not a current correctness bug, but it is the main maintenance risk in the repo: too many changes will cross-cut a single file and make regressions harder to isolate.\n\n5. Batched bootstrap is reasonably covered, but the restart/cutover edge cases are still under-specified. The critical path is internal/syncer/syncer.go, especially the resume and finalization logic around temp refs ( lines 1215-1303). I did not see tests that exercise: temp ref at an unexpected hash, temp ref already at final tip but real ref missing, or partial failure between final create and temp-ref delete. Those are the places most likely to bite operators.\n\n What The Code Gets Right\nThe strategy selection is coherent: Run decides between bootstrap relay, incremental relay, and local-object push in a way that matches the docs. The protocol surface is intentionally narrow. Integration coverage is better than average for transport code, especially the in-process smart HTTP tests and the optional git-http-backend tests in internal/syncer/git_http_backend_test.go. The docs also align unusually well with the implementation.\n\n If Rewriting From Scratch\nI would keep the product boundaries and the relay-first mindset. I would redesign the code around explicit modules: discovery, planning, strategy, transport, auth, and reporting, with a small SyncEngine coordinating them. The current Run/Bootstrap functions are doing too much implicit strategy work; I’d instead produce a typed execution plan first, then execute a named strategy. I’d also isolate protocol-v1/v2 behind a shared source interface so batching, fetch-pack, and ls-refs logic are not spread across orchestration code.\n\nThe migration path would be incremental, not a rewrite: first extract auth and HTTP transport, then move planning into its own package, then turn bootstrap/incremental/local push into separate strategy executors. That would reduce accidental complexity without changing the CLI surface.\n\nIf you want, I can turn this into a concrete review memo with severity labels and a prioritized refactor plan, or I can start fixing the top two issues directly."}} </subagent_notification>

3mo ago·1 call

The protocol-focused review is back as well. It surfaced one concrete correctness issue I hadn’t called out yet: protocol-v2 tag fetches appear to request include-tag without checking server support, unlike the v1 path. I’m waiting on the final core-execution reviewer, then I’ll consolidate everything into one repo overview plus review summary.

<subagent_notification> {"agent_path":"019d7d09-b449-7180-ae04-3b7cfb51b201","status":{"completed":" Findings\n\n1. Batched bootstrap can claim tag refs were pushed even when it never creates them. In internal/syncer/syncer.go, tag creation after branch batches is skipped entirely when FetchPack returns git.NoErrAlreadyUpToDate. That is wrong for lightweight tags, or any tag whose objects are already reachable from the just-pushed branches: no pack is needed, but the ref still must be created with a command-only push. The function then still reports success via result.Pushed = len(plans) at internal/syncer/syncer.go. There does not appear to be a test covering “tag object already present, ref absent”.\n\n2. Duplicate target mappings are silently accepted and overwrite each other. buildDesiredRefs writes desired[targetRef] = ... with no collision check at internal/syncer/syncer.go, so --map main:stable --map release:stable will just keep the last one. That is a correctness issue, not just UX: the effective sync set differs from the user’s request with no error, and prune/planning then operate on the wrong managed target set.\n\n3. Mapping normalization accepts inconsistent ref kinds and partially-qualified refs, then fails later in confusing ways. In internal/syncer/syncer.go, if either side starts with refs/, both sides are treated literally and the kind is inferred only from the target ref. That allows bad combinations like refs/tags/v1:refs/heads/stable or main:refs/heads/stable; the first can explode later when branch planning tries to load a tag object as a commit, and the second produces a misleading “source ref not found” instead of normalizing main to refs/heads/main. This should be rejected during CLI/config validation.\n\n4. Protocol v2 fetch requests always send include-tag for tag syncs without checking whether the server advertised support. See internal/syncer/protocol_v2.go. That differs from the v1 path, which gates on capability support, and it is reinforced by the current test at internal/syncer/protocol_v2_test.go. On a v2 server that supports fetch but not include-tag, tag sync can fail unnecessarily.\n\n Overview\n\nThe repo is small and coherent: cmd/git-sync/main.go is a thin CLI over one main implementation package, internal/syncer. The core design is remote-to-remote smart-HTTP sync using go-git with an in-memory object store, plus two relay optimizations: empty-target bootstrap relay and a narrower incremental relay path. Most of the behavioral surface is concentrated in internal/syncer/syncer.go, with protocol-v2 specifics in internal/syncer/protocol_v2.go.\n\nThe test suite is stronger than the code size suggests: unit tests cover helpers, integration tests cover in-process smart HTTP flows, and git-http-backend tests exercise more realistic end-to-end behavior. go test ./... passes locally.\n\n From Scratch\n\nIf I were rebuilding this, I would split syncer.go into explicit layers: config/validation, planner, source transport, target transport, relay executor, and materialized push executor. Right now too much policy and wire behavior sits in one file, which makes edge cases like batching, tag handling, and mapping validation easy to miss.\n\nI would also make batching resumability explicit instead of inferred from temp refs alone: store checkpoint metadata, validate provenance before resuming, and keep tag creation as a separate post-branch ref-update phase that works even with an empty pack. The other structural change would be to treat capability negotiation as a first-class typed object, so unsupported combinations like v2-without-include-tag are handled centrally instead of ad hoc in request builders.\n\nResidual risk is mostly around capability variation and restart semantics: the happy paths are well tested, but mixed mapping inputs, duplicate config, and odd server capability combinations are not locked down yet. The sub-agent launcher is not available in this session, so I ran the requested review as separate focused passes rather than literal spawned agents."}} </subagent_notification>

3mo ago

Repo overview: this is a small Go CLI centered on remote-to-remote smart-HTTP Git mirroring. The surface area is cmd/git-sync/main.go for flag parsing and output, and almost all real behavior lives in internal/syncer/syncer.go plus protocol-v2 handling in internal/syncer/protocol_v2.go. The main execution modes are normal sync, empty-target bootstrap relay, incremental relay, plus batched bootstrap for large initial migrations. The docs in README.md, docs/bootstrap.md, and docs/bootstrap-batching.md are broadly aligned with the implementation. go test ./... passes.

I reviewed the implementation myself and kicked off focused review passes for:

  1. Core execution and correctness.
  2. Protocol/transport internals.
  3. Test coverage and risk gaps.
  4. “If we rewrote this from scratch, what would we do differently?”

Highest-signal findings:

  1. Batched bootstrap can report tag refs as pushed even when no tag ref was created, if the tag objects are already present and FetchPack returns git.NoErrAlreadyUpToDate in the tag phase. See internal/syncer/syncer.go.
  2. Duplicate target mappings are silently accepted with last-write-wins semantics, which is dangerous for a mirroring tool. See internal/syncer/syncer.go.
  3. Mapping normalization allows inconsistent ref kinds and partially qualified refs, then fails later in confusing ways instead of rejecting them up front. See internal/syncer/syncer.go.
  4. Protocol-v2 tag fetches appear to request include-tag without checking server support, unlike the v1 path. See internal/syncer/protocol_v2.go.
  5. The biggest architectural risk is concentration: internal/syncer/syncer.go mixes planning, strategy selection, batching, push mechanics, auth, token refresh, stats, and measurement in one file.
  6. Tests are good on happy-path end-to-end behavior, especially integration and git-http-backend, but weaker around destructive ref policy, duplicate mappings, batching failure/restart edges, and malformed protocol-v2 input.

From-scratch critique: I would keep the product boundary and the relay-first strategy, but split the code into explicit layers: config/validation, planner, source transport, target transport, strategy executors, and reporting. I would also make batching resume state explicit instead of inferring only from temp refs, and centralize capability negotiation so v1/v2 feature checks are enforced consistently.

If you want, I can turn this into a tighter review memo with severity/priorities, or start fixing the concrete issues above.

put it in a review memo

3mo ago

Review Memo

Scope I reviewed the repository structure, read the implementation and docs, ran go test ./..., and split the review across four focused tracks:

  1. Core execution and correctness.
  2. Protocol and transport internals.
  3. Test coverage and risk gaps.
  4. From-scratch redesign.

Current verification state: go test ./... passes.

Repo Shape This is a compact Go CLI. The entrypoint in cmd/git-sync/main.go is thin; nearly all behavior lives in internal/syncer/syncer.go, with protocol-v2 handling in internal/syncer/protocol_v2.go. The product model is coherent: discover refs, build a plan, then choose one of three execution paths:

The docs in README.md, docs/bootstrap.md, and docs/bootstrap-batching.md match the code reasonably well.

Findings

  1. High: batched bootstrap can claim tag refs were pushed even when they were never created. In internal/syncer/syncer.go, the post-branch tag phase skips work when FetchPack returns git.NoErrAlreadyUpToDate. That is incorrect when the tag object is already reachable from pushed branch history but the tag ref itself does not yet exist. The code then still reports success through result.Pushed = len(plans) at internal/syncer/syncer.go.

  2. High: duplicate target mappings are silently accepted. buildDesiredRefs overwrites desired[targetRef] with no collision detection in internal/syncer/syncer.go. A request like --map main:stable --map release:stable degrades into last-write-wins behavior with no error, which is dangerous for planning and prune semantics.

  3. High: mapping validation is too permissive and fails too late. In internal/syncer/syncer.go, mixed or partially-qualified mappings can survive normalization and only fail later in confusing ways. Examples: tag-to-branch mappings or main:refs/heads/stable. These should be rejected at config validation time.

  4. Medium: protocol-v2 tag fetches appear to request include-tag without checking whether the server advertised support. The v1 path gates this capability, but the v2 request builder does not. See internal/syncer/protocol_v2.go. On stricter servers, valid tag syncs can fail unnecessarily.

  5. Medium: expired EntireDB tokens can be reused after refresh failure, which obscures the real failure mode. In internal/syncer/syncer.go, getEntireDBTokenWithRefresh returns the stale token when refresh fails instead of surfacing a clearer auth-refresh problem.

  6. Medium: the main maintenance risk is architectural concentration. internal/syncer/syncer.go mixes planning, strategy selection, batching, transport, push mechanics, auth, token refresh, stats, and measurement. That is workable now, but it raises the cost of safe changes and makes bugs easier to introduce at boundaries.

Test Assessment The suite is stronger than the repo size suggests. Integration coverage is good, and the optional git-http-backend tests validate more realistic end-to-end behavior. Happy paths are reasonably well covered:

The weakest areas are:

The current suite gives more confidence in transport behavior than in edge-case policy correctness.

What The Code Gets Right The product boundary is good. The relay-first approach is practical and matches the problem. Strategy selection in Run is conceptually sound. The docs are unusually aligned with the implementation, which is valuable for a tool with protocol-specific behavior. The test strategy is appropriately integration-heavy.

From-Scratch View If rebuilding this today, I would keep the same product goal and probably the same three execution modes. I would change the structure:

I would also make capability negotiation and batching resume state explicit types rather than scattered logic. The current design’s main accidental complexity comes from combining protocol, planning, execution, and auth concerns in one package and mostly one file.

Recommended Next Steps

  1. Fix the batched tag-create bug by issuing command-only ref creation when no tag pack is needed.
  2. Reject duplicate target mappings and inconsistent mapping kinds during validation.
  3. Gate protocol-v2 include-tag on advertised support.
  4. Add tests for duplicate mappings, mixed ref-kind mappings, batched tag creation when objects already exist, and batch resume/cutover edge cases.
  5. Start extracting validation and strategy execution into separate modules before doing larger feature work.

If you want, I can turn this into a PR-style issue list next, or start implementing the top fixes.

can you incorporate:

All 5 reviews are in. Here's the compiled report:

git-sync Code Review — 5 Perspectives

1. Code Quality & Correctness

Real bugs found:

Other concerns:

2. Architecture & Design

What's good:sourceRefService cleanly hides v1/v2 behind Fetch/FetchPack. Guard functions (canBootstrapRelay, canIncrementalRelay) return reason strings for diagnostics. Clean public API (Run/Bootstrap/Probe/Fetch + typed results). No global state.

Main concerns:

3. Test Coverage & Quality

Major gaps:

Infra risks:

4. "From Scratch" Redesign

Top 3 changes, in priority order:

  1. Drop go-git's transport layer, own the HTTP layer entirely. git-sync already bypasses go-git for v2 (protocol_v2.go), for relay streaming, and partly for v1 (requestInfoRefs). The result is two parallel HTTP stacks. A custom gitproto package handling pkt-line + smart HTTP for both v1/v2 would eliminate ~30 transitive dependencies and the dual-stack problem. Keep go-git's packfile codec for the decode-repack fallback.

  2. Split into focused packages: gitproto (protocol), planner (ref comparison, checkpoint planning), auth (credential chain), syncer (orchestration). Key interfaces: RefLister, PackFetcher, PackPusher. Makes batch planning testable without HTTP.

  3. Replace progressf with slog: Leveled structured logging (branch=main batch=3/7) instead of ad-hoc stderr formatting. Matches the existing --json output philosophy.

Pack relay design is correct — streaming source→target without decode is the right core insight. The CLI flat Config struct is fine for a tool invoked by automation.

5. Performance & Scalability

Critical issues:

Moderate issues:

3mo ago

Yes. Here’s the revised memo with your compiled 5-review report integrated.

Review Memo

Scope This memo combines:

  1. Code quality and correctness
  2. Architecture and design
  3. Test coverage and quality
  4. From-scratch redesign
  5. Performance and scalability

Repo Shapegit-sync is a compact Go CLI for remote-to-remote smart-HTTP Git mirroring. The entrypoint in cmd/git-sync/main.go is thin. Most behavior lives in internal/syncer/syncer.go, with protocol-v2 logic in internal/syncer/protocol_v2.go.

The execution model is coherent:

The docs in README.md, docs/bootstrap.md, and docs/bootstrap-batching.md broadly match the implementation.

Strengths

Findings

  1. High: batched bootstrap can report tag refs as pushed even when no tag ref was created. In internal/syncer/syncer.go, post-branch tag creation is skipped if FetchPack returns git.NoErrAlreadyUpToDate. That is wrong when objects already exist but the tag ref does not. The result still reports success via internal/syncer/syncer.go.

  2. High: duplicate target mappings are silently accepted. buildDesiredRefs overwrites desired[targetRef] without collision detection in internal/syncer/syncer.go. This is a real correctness issue for planning and prune behavior.

  3. High: mapping validation is too permissive and fails too late. internal/syncer/syncer.go allows inconsistent or partially-qualified mappings that later fail in confusing ways instead of being rejected at validation time.

  4. High: sideband preference is backwards. In buildSidebandIfSupported at internal/syncer/syncer.go, sideband is chosen before sideband64k. Git should prefer sideband64k when available.

  5. High: pack reader leak in the bootstrap batch loop. In internal/syncer/syncer.go, each batch fetches a packReader, but error paths around pushPackToTarget do not guarantee timely close of the response body.

  6. High: statsCollector has a data-race risk. The internal map is mutated from round-trip close callbacks and also read or mutated from the main flow without synchronization around the full access pattern in the statsCollector area near internal/syncer/syncer.go.

  7. Medium: protocol-v2 tag fetches request include-tag without checking whether the server advertised support. See the request builder in internal/syncer/protocol_v2.go. The v1 path gates this capability; the v2 path should too.

  8. Medium: unbounded io.ReadAll on server responses creates avoidable memory-risk exposure. This was called out around protocol_v2.go:713.

  9. Medium: OAuth refresh failures are swallowed and stale tokens are reused. The failure path around internal/syncer/syncer.go degrades into later auth failure with poor diagnostics.

  10. Medium: file-based token storage has no locking. Concurrent processes can corrupt the token file around internal/syncer/syncer.go.

  11. Medium: the probing strategy for batch planning is expensive. sourcePackExceedsLimit at internal/syncer/syncer.go fetches whole packs just to discard them after size probing. That scales poorly.

  12. Medium: the non-relay path fundamentally does not scale well because it materializes fetched objects in memory. Large multi-branch non-relay syncs will hit heap pressure hard.

  13. Medium: reachesCommitHash does unbounded ancestry traversal. In worst cases it can walk a very large history graph at internal/syncer/syncer.go.

  14. Medium: the main maintenance risk is architectural concentration. internal/syncer/syncer.go mixes transport, auth, planning, batching, execution, stats, and measurement in one file.

Architecture Assessment The design is directionally good, but the implementation is under-factored.

Current structural issues:

Natural split points are already visible:

Test Assessment The suite is good on happy-path end-to-end behavior and better than average for protocol-heavy code. Confidence is strongest in:

Main gaps:

From-Scratch Redesign If rebuilding this today, the top changes would be:

  1. Own the Git smart-HTTP protocol layer directly. The code already partially bypasses go-git transport for v2 and relay logic. A dedicated gitproto package for pkt-line and smart HTTP would remove the current dual-stack design while still allowing go-git packfile codecs where useful.

  2. Split the code into focused packages. Recommended split:

  1. Replace ad hoc progress logging with structured logging. A slog-style approach would fit the tool much better than plain stderr formatting, especially given the existing machine-readable output mode.

I would keep:

Performance and Scalability The biggest scalability issue is not one bug; it is the current cost model:

This is acceptable for moderate repos and the current scope, but it is not a design that will age well without refactoring.

Recommended Priorities

  1. Fix correctness bugs first:
  1. Fix concurrency and safety:
  1. Add tests for the currently exposed risk areas:
  1. Start structural refactoring before adding more features:

Bottom Line The product idea is good, and the relay-oriented core is the right foundation. The main problems are concentrated in edge-case correctness, scalability cost, and maintainability. This is not a repo that needs a conceptual rewrite, but it is a repo that now wants deliberate decomposition before more complexity is added.

If you want, I can turn this into:

Let's turn this into an issue list in a markdown file, I'm fine doing a full rewrite in a branch that we can compare to the existing code so we are throwing out wrong things and rebuild properly from scratch and address the issues

3mo ago·1 call

I’m turning the review into a concrete issue list and I’m going to frame it for a rewrite branch, so it separates “must preserve” from “replace” and gives you a comparison checklist against the current implementation.

also put the rewrite memo into a markdown file for future reference

3mo ago·1 call

I’m adding the rewrite memo as a separate reference doc so the issue list stays operational and the architectural rationale stays stable.

Added both docs:

The issue list is execution-oriented. The memo is the longer architectural reference for the rewrite branch. If you want, I can next scaffold the rewrite package layout so the branch starts with the intended structure instead of accreting it later.

can you give me a summary what changed now (did a lot of development on the local branch) compared to original main?

3mo ago·11 calls

I’m checking the branch state against main now and I’ll summarize the substantive development areas rather than dump raw diff output.

The branch is a real rewrite, not an incremental patch set. I’m reading the new package layout and top-level entrypoints so the summary reflects the actual new architecture and behavior changes.

Compared to original main (06fd2be), your local branch soph/rewrite is a substantial rewrite, not a feature-sized diff.

High-level shape:

What changed The biggest change is architectural: the old monolithic internal/syncer/syncer.go has been split into focused packages:

internal/syncer is now mostly orchestration and type re-exports instead of owning protocol, planning, auth, batching, stats, and execution itself.

Behavioral and design changes A few review findings have clearly been addressed in the rewrite:

The protocol-v2 implementation that used to live in internal/syncer/protocol_v2.go has effectively been replaced by internal/gitproto/*. That is the most important design shift: protocol handling is now its own layer instead of embedded inside sync orchestration.

Test and support changes There’s a large increase in direct unit coverage:

Docs were also added for the rewrite effort:

Bottom line Relative to original main, the branch now has:

If you want, I can also turn this into a more PR-style summary grouped as:

  1. architecture changes
  2. behavior changes
  3. test changes
  4. likely regressions / things to review carefully

^what is missing looking at the rewrite docs?

3mo ago·23 calls

I’m comparing the rewrite docs against the current rewrite branch and I’ll call out what still looks unimplemented or only partially addressed, not just restate the issue list.

The docs are now partly reflected in code, but not all the acceptance criteria are proven yet. I’m waiting on the race run because that’s explicitly one of the rewrite-doc success checks.

Looking at the rewrite docs versus the current soph/rewrite branch, a fair amount is already done, but a few of the bigger rewrite goals are still only partially realized.

What still looks missing or incomplete:

What looks done from the docs:

So the short answer is: the rewrite has achieved the package decomposition and fixed several concrete issues, but it has not yet completed the deeper “clean-slate protocol/abstraction/perf” goals from the memo.

If you want, I can do the next pass and annotate rewrite-issue-list.md with done / partial / open based on the current branch.

yes, annotate the list, and keep going, do meaningful commits in the meantime, spawn one or more review agents after each meaingful step

3mo ago·11 calls

I’m updating the rewrite tracker against the actual branch state first, then I’ll turn one or two still-open items into real code/tests so this isn’t just documentation churn. After each meaningful step I’ll commit and run a focused review agent on the result.

I’m reading the rest of the tracker before editing so the status annotations stay consistent all the way through, not just in the first section.

I’ve mapped most tracker items to the current branch. I’m marking them done, partial, or open now and adding a few short notes where the branch intentionally only went halfway.

The tracker is now grounded in the actual branch instead of the original review state. I’m committing that first as a standalone documentation checkpoint, then I’ll run a review agent against the updated tracker before moving to another concrete gap.

The tracker annotation is committed as 4b63c50. I’m kicking off a focused review agent on that doc now to catch any status mismatches before I make the next code change.

I’m taking one still-missing item from the tracker and turning it into real coverage next: context cancellation. That closes a genuine gap and gives the rewrite branch a sharper contract around network abort behavior.

I’m adding cancellation coverage at two levels: the raw smart-HTTP layer and one top-level sync entrypoint. That should close the gap in a way that actually protects the rewrite boundaries.

<subagent_notification> {"agent_path":"019d7e2f-0041-7310-951c-94020254dfc9","status":{"completed":"Findings\n\n1. ### 22. No benchmark coverage for the expensive paths is overstated as done. I only found benchmarks in internal/gitproto/benchmark_test.go and internal/planner/benchmark_test.go. I did not find execution-path benchmarks for bootstrap relay, incremental relay, or the materialized fallback. Given the issue text says “relay path overhead, planning overhead, and fallback graph/object work,” this looks partial, not done.\n\n2. The acceptance criterion Stats and logging are concurrency-safe. Status: partial is directionally right, but the note is imprecise. Stats are clearly synchronized in internal/syncer/stats.go and go test -race ./... passes; logging is still ad hoc progressf in internal/strategy/bootstrap/bootstrap.go. I would either split that criterion into two bullets or say “stats: done, logging: open”.\n\n3. ### 19. Relay eligibility logic is only tested indirectly may still be a bit generous at partial. I found direct tests around mapping/planning and some strategy behavior, but I did not find isolated tests equivalent to the original canIncrementalRelay / fallback-reason decision layer. The rewrite may have improved testability, but the issue text is about direct decision tests, and I don’t see those yet.\n\n4. ### 20. Protocol v2 error handling is under-tested being done is mostly justified, but “unsupported capability combinations” is the weakest part of that claim. The pkt-line and malformed-response coverage is clearly there in internal/gitproto/pktline_test.go and internal/gitproto/fetch_test.go, but I did not verify a broad matrix of unsupported capability combinations beyond targeted checks. If you want the tracker to be strict, this is closer to partial; if you want pragmatic tracking, done is defensible.\n\n5. The tracker does not call out one major remaining memo gap explicitly enough: the rewrite still has not fully eliminated the dual-stack transport design. internal/gitproto is a big step forward, but it still depends on go-git transport/protocol types in internal/gitproto/smarthttp.go. Your acceptance note mentions this, but it is important enough that it may deserve its own open/partial issue rather than only living in the notes.\n\nOverall, most done / partial / open annotations look justified. The two places I would definitely change are:\n- #22 benchmarks from done to partial\n- the combined stats and logging acceptance bullet into separate statuses"}} </subagent_notification>

3mo ago·19 calls

The tracker review came back with two useful corrections: benchmark coverage is only partial, and the “stats and logging” acceptance criterion should not be lumped together. I’m folding those fixes in while I finish the context-cancellation test step and run the suite.

The cancellation tests are close; one import is missing in integration_test.go. I’m fixing that and applying the tracker-review corrections before rerunning tests.

This step now has real branch movement: context-cancellation coverage was added and the tracker was corrected from the review feedback. I’m committing that as the next checkpoint, then I’ll run another review agent on the new tests/status changes.

The cancellation/tracker step is committed as 4e9ecda. I’m sending a review agent over the new tests and tracker adjustments now, then I’ll pick the next still-open rewrite gap if there’s time.

I’m taking one more still-open rewrite item while that review runs: packet-reader allocation churn. It’s a contained protocol-layer improvement and fits the remaining open performance work better than touching strategy behavior next.

I checked the packet reader before changing it and the rewrite already did the obvious buffer reuse work there, so that doc item is less clear-cut than the old review suggested. I’m switching to a better next step: add execution-path benchmarks, because the tracker is right that we still only benchmark protocol/planner internals.

<subagent_notification> {"agent_path":"019d7e30-2f7a-7f60-a942-c59ab0be98d9","status":{"completed":"Two meaningful steps landed, both committed, and the branch is clean.\n\nFirst, I annotated rewrite-issue-list.md against the actual rewrite branch and committed that as 4b63c50 (Annotate rewrite tracker against current branch state). I then ran a focused review pass on the tracker, which surfaced two real corrections: benchmark coverage was only partial, and the combined “stats and logging” acceptance criterion was too coarse. Those corrections are now folded in.\n\nSecond, I addressed one still-open gap from the docs: context cancellation coverage. I added low-level in-flight cancellation tests in internal/gitproto/smarthttp_test.go and a syncer-level propagation test in internal/syncer/integration_test.go, then committed the stronger version as 1a7aefc (Strengthen in-flight cancellation coverage). go test ./... passes after the change.\n\nNet result:\n- tracker is now done / partial / open instead of “everything open”\n- cancellation coverage moved from missing to basic real coverage\n- benchmark and logging status in the tracker is more accurate\n- working tree is clean\n\nThe biggest remaining gaps from the rewrite docs still look like:\n- full elimination of the dual-stack go-git transport dependency\n- stronger abstraction boundaries via explicit interfaces\n- reducing or eliminating fetch-and-discard batch probes\n- structured logging instead of progressf\n- broader failure-path coverage around batch cutover/resume\n- execution-path benchmarks for bootstrap/incremental/materialized strategies\n\nIf you want, I can keep going on the next substantive item and take the same approach: implement, commit, review, repeat."}} </subagent_notification>

3mo ago·31 calls

I’ve closed the cancellation gap. The next low-risk, still-missing rewrite item is benchmark coverage on the fallback/strategy side, and I’m checking whether I can add something meaningful there without faking a whole Git server stack.

I found another clean gap that maps directly to the rewrite tracker: relay-eligibility logic is still under-tested in isolation. I’m adding direct tests for the planner’s relay decision functions instead of only relying on higher-level execution paths.

The relay decision layer is still only partly nailed down. I’m adding direct reason-path tests for CanFullTagCreateRelay, RelayFallbackReason, and a couple of negative CanIncrementalRelay cases so the planner logic is specified without going through full sync flows.

One of the new relay tests exposed a real semantic detail in the rewrite: tag-create-only plans are still classified under the broader incremental relay reason first. I’m correcting the test to reflect the actual decision order rather than forcing a different contract by accident.

The second failure confirms RelayFallbackReason reports the second-stage tag-relay rejection reason, not the first incremental rejection reason, when neither path applies. I’m encoding the current contract explicitly in the test so the behavior is documented rather than implicit.

The relay-decision tests are in and green. I’m updating the tracker once more so it reflects that this gap is now materially closed, not just “improved.”

This step is now coherent: relay-decision coverage is materially stronger, and the tracker reflects that benchmark coverage is still only partial. I’m committing those together as the next checkpoint, then I’ll run one more focused review agent on the planner decision layer.

The relay-decision test step is committed as f9170fc. I’m sending a reviewer over the planner decision logic and the tracker status change now to make sure I didn’t accidentally codify a misleading contract.

The agent limit is full, so I’m closing the earlier review threads before opening a fresh one for this step.

Review the latest changes since commit 4e9ecda, focusing on internal/planner/planner_test.go and the related status change in docs/rewrite-issue-list.md. Check whether the new relay-decision tests are meaningful, whether they lock in the right behavior rather than an accidental quirk, and whether marking issue #19 done is justified. Do not edit files. Return concise findings first with file references.

3mo ago·22.5s·2 calls

I’ve done three concrete rewrite steps this turn and committed each one. I’m checking the branch state once more, and then I’ll give you a compact summary of what landed, what the tracker now says, and what the next highest-value gaps still are.

Three meaningful steps landed and the branch is clean.

What changed:

Current branch state:

What still stands out from the rewrite docs:

I also attempted the requested post-step review-agent pass each time. One tracker review fed back useful corrections that I incorporated. The last agent spawn hit tool friction, so I did not get a final reviewer result back for f9170fc, but the branch state is clean and tested.

Review the latest commit 689eadd on this branch. Focus on the new gitproto.Pusher boundary, strategy Params changes in bootstrap/incremental/materialized, syncer wiring, and the new direct strategy tests. Return findings only, ordered by severity, with file references. Do not make changes.

3mo ago·9.0s·3 calls

I’m reviewing commit 689eadd directly against its parent, focusing on the new gitproto.Pusher boundary, strategy param changes, syncer wiring, and the new direct strategy tests.

  1. High: bootstrap.Execute now only checks TargetPusher for the one-shot path, but the batched path dereferences it unconditionally. If BatchMaxPack > 0 or one-shot bootstrap falls back into batching, executeBatched will call p.TargetPusher.PushPack / PushCommands on a nil interface and panic instead of returning a configuration error. The one-shot nil-check also happens after fetching the pack, so that path leaks the fetched reader on misconfiguration. See internal/strategy/bootstrap/bootstrap.go and the unguarded calls in internal/strategy/bootstrap/bootstrap.go.

  2. Medium: the new strategy tests validate that injected pushers are called, but they do not exercise the new failure mode introduced by the abstraction boundary: missing or invalid TargetPusher. In particular, there is no test covering batched bootstrap with a nil pusher, which is where the current panic path hides. See the new tests in internal/strategy/bootstrap/bootstrap_test.go and internal/strategy/incremental/incremental_test.go.

  3. Medium: the incremental strategy’s new CanRelay injection does not fully represent the previous decision boundary, because it only abstracts CanIncrementalRelay; CanFullTagCreateRelay is still hard-coded internally. That means the new boundary is asymmetric: callers can swap the branch-update eligibility logic, but not the full tag-create relay decision. The new direct tests lock in that asymmetry rather than a clean “strategy policy is injected” model. See internal/strategy/incremental/incremental.go and the new tests in internal/strategy/incremental/incremental_test.go.

  4. Low: gitproto.Pusher is presented as a boundary, but it is still a thin concrete wrapper over Conn + Adv rather than a real decoupling layer. The syncer still constructs it directly from transport state, and the strategies still depend on narrow ad hoc interfaces instead of a stable named abstraction. This is not a correctness bug, but it means the commit improves testability more than it improves actual architectural separation. See internal/gitproto/push.go and the wiring in internal/syncer/syncer.go.

Review commit 3d8ba82 on this branch. Focus on the adaptive bootstrap checkpoint probe-span logic in internal/strategy/bootstrap/bootstrap.go and the new unit coverage in bootstrap_test.go. Return findings only, ordered by severity, with file references. Do not make changes.

3mo ago·7.4s·3 calls

I’m reviewing commit 3d8ba82 against its parent, focusing on the adaptive checkpoint probe-span logic and the new bootstrap unit tests.

No findings.

Review commit 342fb90 (Add repeatable benchmark command). Focus on command ergonomics, whether the initial scenario set is the right minimum baseline, any correctness issues in target setup or URL normalization, and whether the report shape is useful for future optimization work. Concrete findings only.

3mo ago·12.6s·5 calls

I’m reviewing commit 342fb90 against its parent with focus on the benchmark command UX, scenario baseline, target setup/URL normalization, and usefulness of the report output.

I’ve identified a couple of concrete problems in the new benchmark command. I’m grabbing exact line references now so the review is actionable rather than hand-wavy.

I have the main issues. One is a real command-behavior problem, the others are about the benchmark baseline and report usefulness.

  1. Medium: the command exits successfully even if every benchmark run fails. In the main loop, per-run errors are only recorded into runSummary.Error; the command still builds and prints a report, then returns nil. That makes the tool unsafe for automation and easy to misread in CI or scripts. See cmd/git-sync-bench/main.go and the unconditional success return at cmd/git-sync-bench/main.go.

  2. Medium: the initial scenario set is not a useful minimum baseline because sync is currently benchmarked against a freshly initialized empty bare target on every run, so it will usually exercise the same bootstrap-on-empty-target path as bootstrap. That means the two built-in scenarios are not meaningfully separated for optimization work, and the docs already hint at this by saying sync “may choose bootstrap relay automatically” when the target is empty. See the fresh target creation in cmd/git-sync-bench/main.go, the scenario dispatch in cmd/git-sync-bench/main.go, and the scenario description in docs/benchmarking.md.

  3. Low: the aggregate report shape uses sentinel -1 values for min fields when all runs fail, which makes downstream analysis awkward and easy to mishandle. For a benchmarking/reporting tool intended to support future optimization work, “no successful runs” should be represented explicitly rather than via magic numeric values in min_wall_millis and min_sync_elapsed_millis. See initialization in cmd/git-sync-bench/main.go and the absence of a final normalization step in cmd/git-sync-bench/main.go.

  4. Low: normalizeRepoURL treats any string with a parseable non-empty URI scheme as already normalized, which is convenient for real URLs but fragile for platform-specific local path forms. In particular, this will misclassify Windows-style paths like C:\repo.git as a URL with scheme c rather than converting them to a file:// URL. See cmd/git-sync-bench/main.go.

Quick review of commit f82addb (Clean up README structure). Focus on whether the section reshuffle improves scanability without losing important operator guidance, and whether any docs wording now conflicts with current behavior. Concrete findings only.

3mo ago·12.5s·3 calls

I’m reviewing commit f82addb against its parent with focus on readability improvements versus lost guidance, plus any wording that now conflicts with current behavior.

I see one clear documentation regression from the section shuffle and one wording conflict tied to the new benchmark command. I’m checking the exact README wording against the current command behavior.

  1. Medium: the new Quick Start / Commands split improves scanability, but moving all command examples under ## Commands without subheadings makes the section harder to skim than before once a reader is past the first example. The README now presents plan, bootstrap, batching, measurement, probe, and fetch as one long flat sequence of examples, so operator guidance is still present but less chunked by task than it should be. See README.md.

  2. Medium: the benchmark wording now overstates what git-sync-bench gives you as a baseline. The README lists --scenario bootstrap and --scenario sync as the current scenarios, but for fresh empty targets sync will usually exercise the bootstrap path automatically, so the two scenarios are not actually distinct operator baselines. That wording is easy to misread as “bootstrap vs normal sync” when current behavior is closer to “explicit bootstrap vs sync-on-empty-target”. See README.md and the corresponding behavior description in README.md.

  3. Low: the new ## Protocol Notes section is cleaner, but collapsing the old Why Push Stays V1 explanation into a single sentence loses useful operator context. The current wording still says push uses the low-level receive-pack path, but it no longer explains why that is a deliberate design choice rather than a missing feature, which was valuable guidance for readers trying to understand protocol-v2 scope. See README.md.

Quick review of commit c44e996 (Split README details into docs). Focus on whether the README is now the right level of operator-facing detail, and whether docs/testing.md and docs/architecture.md capture the moved material accurately. Concrete findings only.

3mo ago·10.8s·6 calls

I’m reviewing commit c44e996 against its parent, focusing on whether the README now hits the right operator level and whether the new docs/testing.md / docs/architecture.md preserve the moved material accurately.

I found one documentation accuracy problem in the new architecture doc and one operator-guidance regression in the README trim-down. I’m checking the package list against the repo before I return the review.

  1. Medium: the moved TLS override guidance is now in the wrong document. The flags and env vars in docs/testing.md are general runtime/operator settings for self-signed or local targets, not test-only instructions. Moving them out of the README’s main operator-facing path makes that guidance harder to find and misclassifies it as testing-specific.

  2. Low: docs/architecture.md is not a fully accurate capture of the moved architectural material because the package model omits internal/convert, which is part of the current codebase and still participates in strategy/syncer wiring. The rest of the package list is current, but this omission makes the doc read more “idealized target shape” than “current architectural summary.”

Review commit a05bee4 (Harden bootstrap pack close ownership). Focus on whether bootstrap now has a correct and explicit pack-stream ownership model across one-shot, batched, and tag phases, and whether the new tests cover the right failure cases. Concrete findings only.

3mo ago·17.4s·7 calls

I’m reviewing commit a05bee4 against its parent with focus on pack-stream ownership across bootstrap paths and whether the new tests actually cover the failure cases that matter.

I want the surrounding bootstrap code and helper tests before I call this clean; the ownership model depends on whether PushPack already closes streams on success and error.

  1. Medium: the new ownership model is still not fully specified by tests for the tag phase, even though the code now explicitly takes ownership there too. The added coverage exercises one-shot success/error and batched checkpoint error, but I did not find any test that verifies tag-pack closure on either successful tag push or tag-push failure in the len(tagPlans) > 0 path. That leaves one of the three bootstrap stream-owning paths unverified after this change. See the tag-phase close logic in internal/strategy/bootstrap/bootstrap.go and the absence of corresponding tag-close tests in internal/strategy/bootstrap/bootstrap_test.go.

  2. Low: the new tests validate “was closed eventually,” but they do not verify the more important ownership contract that the strategy closes exactly once even when the pusher already closes the stream. The implementation introduces closeOnce specifically to make double-closing benign, but the tests never assert that this wrapper is actually what protects the strategy across one-shot, batch, and tag flows. That means the new tests prove the end state, not the ownership discipline itself. See closeOnce in internal/strategy/bootstrap/bootstrap.go and the close-tracking tests in internal/strategy/bootstrap/bootstrap_test.go.

Quick review of commit ecd3908 (Clarify materialized limit flag semantics). Focus on whether the wording is now accurate and whether the usage/help text stays aligned with the actual CLI surface. Findings only, ordered by severity, with file/line references where possible.

3mo ago·10.9s·4 calls

I’m reviewing commit ecd3908 against its parent, focusing on the wording change and whether the documented/help surface still matches the actual CLI.

I’ve got the flag definition and the help text match points. I’m just pulling exact line references from cmd/git-sync/main.go so the findings are precise.

  1. Low: the new usage/help text hardcodes --materialized-max-objects 500000 instead of deriving it from syncer.DefaultMaterializedMaxObjects, so the help surface can silently drift from the actual CLI default if the code changes later. See the flag definition in cmd/git-sync/main.go versus the hardcoded usage string in cmd/git-sync/main.go.

  2. Low: the help text now lists --materialized-max-objects under combined sync/plan flags, but the wording in the README correctly describes it as a runtime guard for the materialized fallback path during sync. plan accepts the flag only because it shares the parser path, not because it has any effect there, so the help surface is technically aligned with the parser but not with meaningful behavior. See the shared flag registration in cmd/git-sync/main.go and the README wording in README.md.

Review commit c676521 (Cover returned fetch-pack reader interruptions). Focus on whether the new v1/v2 interruption tests are realistic, whether the updated tracker note is fair, and whether issue #5 now looks substantially closed in code. Findings only, ordered by severity, with file/line references where possible.

3mo ago·10.9s·10 calls

I’m reviewing commit c676521 against its parent, focusing on the new fetch-pack interruption tests, the tracker note change, and whether the underlying stream-ownership issue now looks materially closed.

I’ve got the diff. I’m checking the fetch-pack implementations to see whether these tests match the real reader lifecycle and whether the tracker note is now too optimistic or about right.

Reviewing c676521 now. I’m checking the new fetch interruption tests against the actual v1/v2 reader ownership paths and the tracker note before I conclude whether issue #5 is genuinely closed.

No findings.

Review commit b718e82 ("Add batched planning backend sensitivity test") in /Users/soph/Work/entire/devenv/git-sync. Focus on whether the new git-http-backend test is a sound experiment surface for batching, whether the assertions are meaningful without being flaky, and whether there are any setup mistakes in the source transport/planning wiring. Report only concrete findings with file/line refs if any; otherwise say no findings.

3mo ago·6.7s·9 calls

Reviewing b718e82 now. I’m checking the new git-http-backend batching test for wiring correctness, signal quality, and whether the assertions depend on timing or environment quirks.

The new test landed in internal/syncer/git_http_backend_test.go, not the older integration file. I’m reading that directly and checking the tracker note against the exact assertions.

I’ve got the new test file open. Next I’m reading the added batching-planning case in detail and checking whether it actually isolates backend sensitivity rather than restating the existing unit tests through a heavier harness.

No findings.

Review commit 359d4e9 ("Add materialized object handling examples") in /Users/soph/Work/entire/devenv/git-sync. Focus on whether the new tests actually illustrate materialized object handling correctly, especially the distinction between branch-update object closure, target-have cutoff, and delete-only ref operations. Report only concrete findings with file/line refs if any; otherwise say no findings.

3mo ago·11.7s·4 calls

Reviewing 359d4e9 now. I’m checking whether the new examples really map to the materialized object-push behavior they claim to document, especially around close ownership, target-have cutoffs, and delete-only commands.

  1. Medium: TestCollectObjectClosureForBranchUpdateExample does not actually exercise a branch update case. It sets ActionCreate and provides no TargetHash or TargetRefs baseline, so it illustrates “full closure for a created branch tip,” not “branch-update object handling.” That makes the example misleading relative to its stated purpose and overlaps with the separate target-have cutoff test instead of contrasting with it. See internal/strategy/materialized/materialized_test.go.

Review the current worktree changes adding HTTPClient injection through pkg/gitsync into internal/syncer. Focus on: 1) whether the injected client precedence/semantics are sane, 2) whether wrapping the client's transport for stats is safe, 3) any missing tests or obvious edge-case bugs in the new connection setup. Findings only, ordered by severity with file references.

3mo ago·10.9s·14 calls

Reviewing the current worktree changes now. I’m tracing the new HTTPClient path from pkg/gitsync into internal/syncer, then checking transport wrapping and test coverage around connection setup.

The config path is straightforward. I’m looking more closely at the transport layer now, because the real risk is whether a caller-supplied client keeps its intended semantics once syncer clones it and wraps the transport.

I’ve narrowed it to two areas: the interaction between SkipTLSVerify and a caller-supplied client, and whether the wrapped client is consistently the one used across both the custom smart-HTTP path and any remaining go-git transport path.

One remaining check: I’m comparing the worktree change against the transport constructor in gitproto, because the test story is only convincing if the injected client reaches both smart-HTTP requests and the go-git transport wrapper the same way.

  1. Medium: SkipTLSVerify is effectively ignored when a caller injects an HTTPClient with a non-nil Transport. instrumentHTTPClient only applies gitproto.NewHTTPTransport(skipTLS) when the injected client is nil or has Transport == nil, so endpoint-level TLS override silently stops working once a custom transport is provided. That precedence is not obvious from the API, and it also means source and target cannot honor different SkipTLSVerify settings when they share one injected client. See internal/syncer/syncer.go and the single-client wiring in pkg/gitsync/client.go.

  2. Low: the new tests only prove config plumbing and shallow transport wrapping; they do not cover the important edge semantics introduced by this change. There is no test for HTTPClient + SkipTLSVerify precedence, no test that a nil-transport injected client gets the expected fallback transport, and no end-to-end assertion that the injected client is actually used on both source and target request paths after newSession wiring. See internal/syncer/auth_test.go and pkg/gitsync/client_test.go.

Review the current pkg/gitsync result-shaping changes. Focus on whether the new stable Ref/Counts/Execution grouping is a better public contract than the previous flat engine-shaped fields, and whether any obviously useful worker-facing information was lost or still leaks too much engine detail. Findings only, ordered by severity with file references.

3mo ago·6.6s·4 calls

Reviewing the current pkg/gitsync result-shaping changes now. I’m comparing the new public response shape against the old flat engine-shaped fields, then checking whether the remapped data drops anything callers would still need.

I’ve got the shape diff. I’m checking whether anything practical disappeared for consumers, especially batch-resume and per-run diagnostics that may still matter even if they’re engine-flavored.

  1. Medium: TempRefs disappeared from the public SyncResult, but that is still useful worker-facing state for batched bootstrap observability and recovery workflows. Internally the engine still treats temp refs as meaningful execution output, and tests assert on them directly, so removing them entirely from the public contract throws away actionable information rather than just hiding implementation detail. See pkg/gitsync/types.go versus internal/syncer/syncer.go and the syncer tests in internal/syncer/git_http_backend_test.go.

  2. Low: the new grouping is directionally better than the old flat shape, but Execution.Mode and especially Execution.Reason still leak engine-internal vocabulary directly from RelayMode and RelayReason. That means the public result is only partially stabilized: field grouping improved, but the value space is still coupled to internal planner/syncer phrasing. See pkg/gitsync/types.go and the direct passthrough in pkg/gitsync/types.go.

d456614Add CI workflows and fix all golangci-lint violations\ \ Add GitHub Actions PR gates matching the CLI repo pattern:\ - ci.yml: tests with -race detection on PRs and push to main\ - lint.yml: golangci-lint (60+ linters) with inline PR annotations,\ gofmt, go mod tidy, and shellcheck checks\ - license-check.yml: reusable license compliance from entireio/shared\ \ Add .golangci.yaml with full linter config adapted from CLI, and\ mise-tasks/lint/ scripts for local development parity.\ \ Fix all 340 existing lint violations across 51 files:\ - wrapcheck: wrap external/interface errors with descriptive context\ - perfsprint: errors.New for static strings, string concat for Sprintf\ - errcheck: handle or explicitly acknowledge error returns\ - intrange: modernize for loops to range-over-int (Go 1.22+)\ - embeddedstructfieldcheck: separate embedded from regular struct fields\ - inamedparam: name interface method parameters\ - exhaustive: add missing switch cases\ - errorlint: use errors.Is() instead of == for wrapped error checks\ - revive: rename unused parameters to _\ - goconst: extract repeated string literals into constants\ - gocritic: restructure if-else chains and duplicate branches\ - noctx: use CommandContext/NewRequestWithContext\ - staticcheck: replace nil contexts with t.Context()\ - usestdlibvars: use http.MethodGet instead of "GET"\ \ Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 79aa4e9d44f3+891/-434