Review the staged git changes (git diff --staged main) in this Go repository. This is a rewrite of a git-sync tool that mirrors Git repos over smart HTTP. The rewrite breaks a monolithic syncer.go (31

I’m reviewing the staged rewrite against main, starting with the issue list and the staged diff to identify correctness risks, regressions, and architectural problems before I drill into concurrency behavior.

I have the rewrite scope and staged file set. Next I’m reading the new package boundaries and the remaining issue list sections, then I’ll compare the new orchestration paths against the old behavior where the risky logic used to live.

The new packages are cleaner than the old layout, but I need to verify that the orchestration still preserves all edge cases from the original paths. I’m reading the planner/bootstrap/auth integration points now, because most likely regressions are where the slim orchestrator stitches these pieces together.

I’ve reached the point where the review hinges on exact edge conditions rather than package shape. I’m checking the protocol parsing, push path, and the tests added for the rewrite to see which rewrite issues were actually locked down versus only addressed by code comments.

I’ve read enough to start forming findings, but I want one pass of execution signal before I finalize severity. I’m running the focused test set and scanning the old main implementation for the specific paths that were supposed to be preserved or fixed.

I’ve isolated a few concrete gaps now. I’m pulling exact line references for the failure modes so the findings are specific and defensible rather than just high-level concerns.

Findings:

1. High: EntireDB refresh failures are still effectively swallowed instead of surfacing an auth-refresh error. getTokenWithRefresh now returns a real error, but internal/auth/entiredb.go converts any refresh/read failure into ( "", false ), and internal/auth/auth.go treats that as “no EntireDB credential” and falls through to git credential helper or anonymous auth. That misses the rewrite requirement in issue #7: an expired token plus refresh failure still won’t produce an explicit refresh error path.

2. Medium: Mapping validation still happens after remote I/O, so issues #2/#3 are not fixed at the entry-point level. In Run, Bootstrap, Probe, and Fetch, the code builds ref-prefixes and calls the remotes before planner.BuildDesiredRefs performs ValidateMappings; representative path: internal/syncer/syncer.go then internal/syncer/syncer.go. The planner comment in internal/planner/planner.go claims this happens before network activity, but it does not. --map main:stable --map release:stable will still hit the network before failing.

3. Medium: Duplicate-target protection is incomplete; --include-tags can silently overwrite a mapped target ref. ValidateMappings only deduplicates entries inside cfg.Mappings, but BuildDesiredRefs later writes directly into desired[targetRef] / managed[targetRef] and then auto-adds tags with the same blind assignment (internal/planner/planner.go, internal/planner/planner.go). Example: map refs/tags/release -> refs/tags/v1 with --include-tags and an existing source refs/tags/v1; the automatic tag add will overwrite the explicit mapping with no error.

4. Medium: Issue #9 is only partially fixed; v2 ls-refs still buffers an unbounded response in memory. RequestInfoRefs is capped, but internal/gitproto/refs.go calls internal/gitproto/smarthttp.go, and PostRPC still does a raw io.ReadAll(reader). A malicious or pathological ls-refs response can still force unbounded allocation on a normal control path.

5. Low: --verbose push progress regressed. All push paths set req.Progress = progressWriter(verbose), but internal/gitproto/push.go always returns nil, so verbose mode no longer surfaces receive-pack progress.

Open questions / assumptions:

• I did not find a concrete in-process race in the rewrite; go test -race ./internal/... passed, and the new stats collector looks properly synchronized.
• The package split is materially better than main, but internal/syncer still owns bootstrap strategy, retry heuristics, and GitHub preflight logic, so the “strategy packages + slim orchestrator” goal is only partially realized.
• The file token store is still not process-safe despite the earlier comment claiming locking; the implementation/comment mismatch is visible in internal/auth/tokenstore.go and internal/auth/tokenstore.go.

Verification:

• Ran go test ./internal/...
• Ran go test -race ./internal/...