Review the latest changes since commit e9b6ed4, focusing on the new batched-bootstrap resume/cutover integration tests in internal/syncer/integration_test.go and the related tracker update. Check whet

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.

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.

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.

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.

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.

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:

• Bootstrap relay for empty targets.
• Incremental relay for a narrow safe subset.
• Local materialize-and-push for everything else.

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:

• Empty-target bootstrap relay.
• Incremental relay.
• Branch mapping.
• Tag creation.
• Force/prune behavior.
• Protocol-v2 fetch/probe.
• Batching and resume.

The weakest areas are:

• Destructive ref policy and prune scope.
• Duplicate or conflicting mappings.
• Batched-bootstrap failure and cutover edges.
• Protocol-v2 malformed input and capability variation.
• CLI validation behavior.

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:

• config/validation
• planner
• source transport
• target transport
• strategy/bootstrap
• strategy/incremental
• strategy/materialized
• auth
• reporting

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.

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

Review Memo

Scope This memo combines:

• Direct code review of the repo structure, docs, and implementation.
• Local verification via go test ./... which currently passes.
• Five review perspectives:
  1. Code quality and correctness
  2. Architecture and design
  3. Test coverage and quality
  4. From-scratch redesign
  5. Performance and scalability

Repo Shape git-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:

• bootstrap for empty-target relay
• sync for normal reconciliation
• incremental relay for narrow safe cases
• decode/repack fallback for non-relay cases
• batched bootstrap for very large initial syncs

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

Strengths

• The core product idea is right: source-to-target pack relay without local decode is the key architectural insight.
• sourceRefService is a decent abstraction over protocol v1/v2 fetch behavior.
• Guard functions such as relay eligibility checks return useful reasons, which helps diagnostics.
• Public API shape is clean: Run, Bootstrap, Probe, Fetch with typed result structs.
• Test coverage is stronger than the repo size suggests, especially integration and optional git-http-backend coverage.

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:

• syncer.go is effectively doing transport, auth, planning, push, batching, stats, and measurement.
• Run, Bootstrap, Probe, and Fetch repeat session setup work.
• Several functions carry too many parameters, especially bootstrap helpers.
• Auth and transport behavior are intertwined with orchestration.
• Result/reporting shape is growing alongside execution complexity.

Natural split points are already visible:

• auth
• transport or gitproto
• planner
• strategy/bootstrap
• strategy/incremental
• strategy/materialized
• reporting

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:

• bootstrap relay
• incremental relay
• branch mapping
• tag creation
• prune/force basics
• protocol-v2 probe/fetch
• git-http-backend integration coverage

Main gaps:

• No direct unit coverage for many pure or nearly-pure planning functions.
• Relay path selection is mostly only tested indirectly.
• Protocol-v2 malformed input and truncation paths are largely untested.
• Duplicate/conflicting mappings are untested.
• Empty source repo and tag force-retarget coverage is missing.
• No context cancellation tests.
• No benchmarks for the most performance-sensitive paths.
• Some test infrastructure uses t.Fatalf inside handlers/goroutines, which can fail badly.

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:

• gitproto
• planner
• auth
• syncer orchestration
• strategy executors behind interfaces like RefLister, PackFetcher, PackPusher

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:

• the relay-first strategy
• the flat CLI config shape
• the distinction between bootstrap, incremental relay, and fallback paths

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

• batch planning does repeated full-pack probe fetches
• fallback path keeps everything in memory
• ancestry checks are graph walks with no cost bounds
• packet parsing allocates frequently
• measurement sampling is aggressive when enabled
• v1 paths may not consistently benefit from the custom HTTP client behavior

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:

• batched tag-create no-op reporting
• duplicate mapping rejection
• mapping validation
• sideband64k preference
• packReader close discipline
• protocol-v2 include-tag capability gating

1. Fix concurrency and safety:

• synchronize statsCollector
• bound large response reads
• stop silently reusing expired tokens after refresh failure
• add file locking for token store if that mode is meant to be multi-process safe

1. Add tests for the currently exposed risk areas:

• duplicate mappings
• invalid mixed-kind mappings
• tag create with already-present objects
• batch resume/cutover failure cases
• malformed/truncated protocol-v2 responses
• empty source repo
• context cancellation

1. Start structural refactoring before adding more features:

• extract validation
• extract auth
• extract transport/protocol
• move execution modes into separate strategy executors

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:

• a shorter exec-summary memo
• a GitHub issue list
• or a prioritized implementation plan with concrete file-level changes.

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.

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:

• rewrite-issue-list.md
• rewrite-memo.md

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.

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:

• 44 files changed
• about 8442 insertions / 3969 deletions
• current branch tip: 025234e
• commits on top of main:
  • 0520b7c Rewrite git-sync into focused packages
  • cd8b183 Add unit tests for strategy packages, auth token flow, and object push
  • 025234e Deduplicate shared helpers and remove dead code

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

• internal/auth
• internal/gitproto
• internal/planner
• internal/strategy/bootstrap
• internal/strategy/incremental
• internal/strategy/materialized

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:

• mapping validation moved into planner, with duplicate target mappings rejected up front in internal/planner/planner.go
• protocol-v2 include-tag is now capability-gated in internal/gitproto/fetch.go
• auth handling was extracted and refresh failures are surfaced explicitly in internal/auth/auth.go
• batched bootstrap tag handling was fixed so tag refs can still be created even if no pack transfer is needed, in internal/strategy/bootstrap/bootstrap.go

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:

• new tests across internal/auth, internal/gitproto, internal/planner, and all three strategy packages
• new benchmarks in internal/gitproto/benchmark_test.go and internal/planner/benchmark_test.go
• existing syncer/integration tests were adjusted rather than removed

Docs were also added for the rewrite effort:

• docs/rewrite-memo.md
• docs/rewrite-issue-list.md

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

• a new package architecture
• a custom protocol layer under gitproto
• extracted planning/auth/strategy logic
• targeted fixes for several known review issues
• much broader unit-test coverage
• rewrite documentation for future comparison

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

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:

• The docs are not reconciled with the branch yet.

  • docs/rewrite-issue-list.md still marks everything as open, even though several items are now clearly addressed.
  • Before using it as a tracker, it needs a pass to mark done, partial, not started.

• The “own the HTTP/protocol layer entirely” goal is only partial.

  • internal/gitproto now exists and does more of the protocol work, but it still leans on go-git transport and protocol types like transport.Endpoint, transport.Transport, transporthttp.NewClient, and packp.
  • So the dual-stack problem is reduced, not eliminated.

• The “recommended interfaces” from the memo are not really in place yet.

  • I don’t see explicit interfaces like RefLister, PackFetcher, PackPusher, Planner, CheckpointPlanner.
  • The package split is there, but the abstraction boundaries are still mostly concrete-function based.

• Batch planning is still using fetch-and-discard probing.

  • It is improved with an initial heuristic in internal/strategy/bootstrap/bootstrap.go, but packExceedsLimit still fetches packs and discards them in internal/strategy/bootstrap/bootstrap.go.
  • So issue #14 is only partially addressed, not solved.

• Structured logging is not adopted.

  • The memo suggested replacing ad hoc progress logging with slog, but the branch still uses progressf in bootstrap paths.
  • Search shows no slog usage.

• Context-cancellation coverage still appears missing.

  • I couldn’t find cancellation-focused tests in *_test.go.
  • The docs explicitly called this out as a missing area.

• The rewrite still keeps the materialized fallback path as an in-memory path.

  • That may be an acceptable product choice, but the deeper scalability question from the memo is not really resolved, only compartmentalized into internal/strategy/materialized.

What looks done from the docs:

• package split away from the old monolith
• shared session/setup extraction in syncer
• duplicate mapping rejection
• up-front mapping validation
• protocol-v2 include-tag capability gating
• race-safe stats collection
• bounded buffered reads in smart HTTP helpers
• file token-store locking
• benchmarks added
• go test -race ./... passes

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.

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.

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.

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.

I’ve done three concrete rewrite-track steps now: tracker annotation, cancellation coverage, and direct relay-decision coverage. I’m just checking the branch state and latest commits before I hand you a concise summary.

Three meaningful steps landed, all committed, and the branch is clean.

I added rewrite tracking in rewrite-issue-list.md so it now reflects the actual branch state with done / partial / open instead of treating everything as still unresolved. That includes correcting benchmark coverage to partial and splitting stats from logging.

I then added cancellation coverage in internal/gitproto/smarthttp_test.go and internal/syncer/integration_test.go, so the rewrite now has direct tests for canceled requests at both the protocol layer and a top-level syncer entrypoint.

After that I added direct relay-decision coverage in internal/planner/planner_test.go, which closes most of the old “relay eligibility is only tested indirectly” gap. The tracker was updated to reflect that.

Latest commits:

• f9170fc Add direct relay decision coverage
• 1a7aefc Strengthen in-flight cancellation coverage
• 4e9ecda Add cancellation coverage and tighten rewrite tracker
• 4b63c50 Annotate rewrite tracker against current branch state

Verification:

• go test ./...
• go test -race ./...

Still clearly open from the rewrite docs:

• fully removing the remaining go-git transport dependency
• stronger interface boundaries
• eliminating fetch-and-discard batch probes
• replacing progressf with structured logging
• harder batch failure/cutover coverage
• execution-path benchmarks for bootstrap/incremental/materialized

If you want, I can keep pushing on the next substantial item directly. My recommendation is either structured logging or batch planning probe reduction, depending on whether you want maintainability or performance next.

I’m moving to the next still-open rewrite item: structured logging. It’s scoped enough to finish cleanly, and since the only remaining ad hoc progress logging is in bootstrap, this should let us actually close that tracker item rather than just improve it.

I’ve confirmed the logging scope is still just bootstrap, which makes this a good candidate to close cleanly. I’m replacing the ad hoc stderr formatter there with slog and wiring it from the top-level syncer session.

I’m replacing the remaining bootstrap progressf calls with structured slog output and wiring a verbose logger from syncer. Since bootstrap is the only place still doing ad hoc progress logging, this should let us close the logging item rather than leave it hanging.

The structured logging change is in place. I’m running the affected packages and then the full suite; if that passes, I’ll update the tracker and commit it as the next rewrite step.

Bootstrap logging is now structured and the suite is green. I’m marking the tracker accordingly, then I’ll commit this as the next meaningful rewrite checkpoint and run a review pass on the logging change.

The structured logging step is committed as d11c597. I’m sending a focused reviewer over it now, then I’ll keep moving on the next substantial open item.

I’m tackling the benchmark gap next, but at the execution-path level instead of adding more microbenchmarks. I’m wiring a few coarse syncer benchmarks around bootstrap, incremental relay, and a materialized fallback case so the tracker’s “partial” on benchmarks is less hand-wavy.

The benchmark gap is best closed at the syncer layer, where the strategies are actually selected. I’m adding coarse benchmarks for empty-target bootstrap, incremental relay, and a forced materialized fallback case, using the existing in-process smart-HTTP fixtures so they’re real execution-path measurements rather than synthetic helpers.

The benchmark step needs one cleanup before it’s real: the first cut reused testing.T helpers in a way that’s too brittle. I’m replacing that with benchmark-native setup and explicit repo-copy helpers so the execution-path benchmarks are actually trustworthy.

The benchmark scaffolding is in. I’m running a one-iteration smoke benchmark, not just go test, so I can catch runtime fixture problems before I claim the execution-path coverage exists.

The execution-path benchmarks are real now and they run successfully. I’m updating the tracker from partial to done on benchmark coverage, then I’ll commit that benchmark step and send a reviewer over it.

The benchmark step is committed as e9b6ed4. I’m sending a focused reviewer over the new benchmark file now to catch anything misleading in the setup or any obvious benchmark-smell before I move to the remaining harder open items.

I’m using the next open test gap now: batched-bootstrap resume/cutover edge cases. I can cover two real states without touching production logic: mismatched temp refs and “temp ref already at final tip, real ref missing” cutover.

I’m adding two batched-bootstrap integration tests now: one for a mismatched resume temp ref failing fast, and one for the “temp ref already at final tip, real ref missing” cutover path succeeding. That should materially reduce the remaining resume/cutover gap in the tracker.

The new resume/cutover tests are in. I’m running the syncer package first since these hit the heavier bootstrap paths, then I’ll update the tracker if they land cleanly.

The mismatch test is failing for the right reason: I accidentally gave the target a real managed branch, so bootstrap rejected before it even reached resume validation. I’m fixing the fixture so the target only has an unrelated temp ref, which is the actual state we need to test.

The new batched-bootstrap resume tests are green. I’m updating the tracker note so it reflects that resume mismatch and final-tip cutover are now covered, while leaving the harder failure injection cases as still open.

The resume/cutover coverage step is committed as 4b7a223. I’m sending a focused review pass over those new integration tests now, then I’ll reassess what remains open versus what is realistically still structural and not worth forcing in this branch.

I’ve pushed the branch through another real gap and the remaining items are getting more structural. I’m checking the tracker state now so I can decide whether the next step should be another concrete fix or whether what remains needs a larger design pass rather than more tactical edits.

The most actionable remaining correctness item is the old batched-tag gap. I can close that with a direct test for the exact case we were worried about: a lightweight tag whose object is already present on target after the branch batches, so tag creation must succeed without needing another pack.

I’ve added the direct lightweight-tag bootstrap case. I’m running that targeted test first, because if it passes it’s strong evidence the old tag-create bug is actually closed in behavior, not just in code shape.

The lightweight-tag bootstrap case is green, so the old batched tag-creation bug now has direct behavioral coverage. I’m marking that issue done in the tracker and committing it as the next checkpoint.

The batched lightweight-tag coverage is committed as f5532bf. I’m sending a final focused review over that edge case now, then I’ll reassess the tracker because the remaining items are increasingly structural rather than “missing test/fix” class issues.

More landed. The branch is clean.

New commits:

• f5532bf Cover lightweight tag creation in batched bootstrap
• 4b7a223 Add batched bootstrap resume edge-case coverage
• e9b6ed4 Add execution-path syncer benchmarks
• d11c597 Replace bootstrap progress logging with slog
• f9170fc Add direct relay decision coverage
• 1a7aefc Strengthen in-flight cancellation coverage
• 4e9ecda Add cancellation coverage and tighten rewrite tracker
• 4b63c50 Annotate rewrite tracker against current branch state

What changed in this pass:

• The rewrite tracker in docs/rewrite-issue-list.md now reflects actual branch state instead of treating everything as still open.
• Cancellation coverage was added at both the protocol layer and the syncer entrypoint layer.
• Relay decision logic now has direct planner-level tests instead of only indirect execution coverage.
• Bootstrap progress logging was replaced with structured slog.
• Syncer-level execution benchmarks now exist for:
  • empty-target bootstrap
  • incremental relay
  • materialized fallback
• Batched bootstrap now has direct integration coverage for:
  • resume mismatch failure
  • final-tip temp-ref cutover
  • lightweight tag creation when no extra pack is needed

Verification:

• go test ./...
• go test -race ./...
• benchmark smoke run for the new syncer benchmarks

What is still genuinely not done:

• full removal of the remaining go-git transport dependency
• stronger interface boundaries instead of broad concrete params
• eliminating fetch-and-discard batch sizing probes
• redesigning the materialized fallback so it scales beyond in-memory limits
• a tighter stream-lifecycle audit around error-path close behavior
• a few harder failure-injection cases around partial cutover

At this point the remaining items are mostly architectural rather than “missing test/fix” class work. The branch is in much better shape, but it is not fully “done” in the rewrite-memo sense yet.