Review docs/rewrite-issue-list.md against the current rewrite branch. Check whether each done/partial/open annotation looks justified by the current code and tests. Focus on factual mismatches, overst

Codex·GPT-5.4·

Soph·5mo ago·1min·1 Checkpoint·3 file changes·+80/-3·4.5M tokens

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:

Core execution and correctness: planning, relay selection, bootstrap, batching, push/fallback behavior.
Protocol and transport internals: pkt-line handling, v1/v2 negotiation, stream safety, HTTP/auth behavior.
Test strategy and gaps: what behavior is actually locked down, and what regressions still look plausible.
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.

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:

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

Highest-signal findings:

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.
Duplicate target mappings are silently accepted with last-write-wins semantics, which is dangerous for a mirroring tool. See internal/syncer/syncer.go.
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.
Protocol-v2 tag fetches appear to request include-tag without checking server support, unlike the v1 path. See internal/syncer/protocol_v2.go.
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.
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:

Core execution and correctness.
Protocol and transport internals.
Test coverage and risk gaps.
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

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.
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.
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.
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.
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.
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

Fix the batched tag-create bug by issuing command-only ref creation when no tag pack is needed.
Reject duplicate target mappings and inconsistent mapping kinds during validation.
Gate protocol-v2 include-tag on advertised support.
Add tests for duplicate mappings, mixed ref-kind mappings, batched tag creation when objects already exist, and batch resume/cutover edge cases.
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:

Sideband priority is backwards (syncer.go:3022-3026): buildSidebandIfSupported checks Sideband before Sideband64k. Per Git protocol, 64k is preferred — the current code picks the lower-bandwidth variant even when 64k is available on both sides.
Pack reader leak in batch loop (syncer.go:1257-1263): In bootstrapBatchedWithInputs, each iteration fetches a packReader but never defers its close. If pushPackToTarget errors before ReceivePack finishes, the HTTP response body leaks.
Data race in statsCollector (syncer.go:3036-3088): The items map is mutated from countingRoundTripper.RoundTrip (HTTP goroutine via onClose) and read by the main goroutine (snapshot(), addWantsHaves()). No mutex protects it.

Other concerns:

Unbounded io.ReadAll on server responses (protocol_v2.go:713) — a malicious server could OOM the process.
Swallowed refresh error (syncer.go:2789): When OAuth token refresh fails, the stale token is silently returned with no logging.
File token store has no locking (syncer.go:2931): Concurrent processes can corrupt tokens.json.

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:

syncer.go is doing 6 jobs in 3143 lines: transport, auth (~350 lines), planning, push (3 variants), bootstrap batching, stats. Natural split points exist with minimal cross-coupling.
Repeated setup across entry points: Run(), Bootstrap(), Probe(), Fetch() all repeat: validate protocol → create stats → create connections → list refs. Needs a shared session/setup struct.
10+ parameter functions: bootstrapBatchedWithInputs takes 11 args. A "sync session" struct would clean this up.
Bootstrap() duplicates Run()'s setup (lines 569-624 mirror 397-466).
Growth risks: Auth chain is 4 strategies deep, relay decision tree has 4 branches, Result struct has 14 fields — all will grow without structural refactoring.

3. Test Coverage & Quality

Major gaps:

No unit tests for many complex pure functions: buildDesiredRefs, buildPlans, planRef, objectsToPush, collectPushObjects, firstParentChain, normalizeMapping, autoBatchMaxPackBytes, bootstrapResumeIndex (error path).
Relay path selection untested in isolation: canIncrementalRelay, canFullTagCreateRelay, relayFallbackReason — only exercised implicitly.
No protocol v2 error handling tests: Malformed server responses, truncated packets, missing version 2 line — all have explicit error returns that are never tested.
Empty source repo never tested. Tag force-retarget never tested.
No benchmarks at all — the relay path and collectPushObjects are performance-sensitive.
No context cancellation tests.

Infra risks:

t.Fatalf used inside HTTP handler goroutines → can hang instead of failing cleanly.
Test helpers (newSourceRepo, makeCommits) duplicated between main_test.go and integration_test.go.

4. "From Scratch" Redesign

Top 3 changes, in priority order:

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.
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.
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:

The probing problem: sourcePackExceedsLimit (syncer.go:1601) fetches the entire pack from source just to check if it exceeds a size limit, then discards all bytes. With ~5 probes per checkpoint, a 10-checkpoint branch does up to 50 full pack fetches thrown away. Fix: reuse a pack that fits instead of re-fetching, or use object count from pack header as a heuristic.
In-memory object store doesn't scale: The non-relay fallback path materializes every fetched object in heap. A 1GB repo needs >1GB heap. Worst case is multi-branch non-relay sync on a large repo.
Unbounded graph walking: reachesCommitHash (syncer.go:2533) does BFS through entire commit ancestry with no depth limit. A force-push scenario on a 100K-commit repo visits every commit.

Moderate issues:

Allocation churn in pkt-line reader (protocol_v2.go:76): Two make([]byte, ...) per packet. Thousands of packets per fetch = significant GC pressure. Reusable buffer would help.
ReadMemStats every 10ms causes ~100 STW pauses/second — acceptable since opt-in, but worth noting.
go-git v1 sessions bypass the custom http.Client, so stats counting and TLS skip may not apply on v1 paths.
Batching overhead: 2N+2 HTTP round-trips for N batches vs. 2 for single push, plus repeated server-side pack computation.

5mo ago

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

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.
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.
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.
High: sideband preference is backwards. In buildSidebandIfSupported at internal/syncer/syncer.go, sideband is chosen before sideband64k. Git should prefer sideband64k when available.
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.
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.
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.
Medium: unbounded io.ReadAll on server responses creates avoidable memory-risk exposure. This was called out around protocol_v2.go:713.
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.
Medium: file-based token storage has no locking. Concurrent processes can corrupt the token file around internal/syncer/syncer.go.
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.
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.
Medium: reachesCommitHash does unbounded ancestry traversal. In worst cases it can walk a very large history graph at internal/syncer/syncer.go.
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:

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.
Split the code into focused packages. Recommended split:

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

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

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

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

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

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 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:

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:

architecture changes
behavior changes
test changes
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 checking the annotated tracker against the current branch state and the relevant tests now.

Findings

### 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.
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”.
### 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.
### 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.
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.

Overall, most done / partial / open annotations look justified. The two places I would definitely change are:

#22 benchmarks from done to partial
the combined stats and logging acceptance bullet into separate statuses