Review the code changes against the base branch 'main'. The merge base commit for this comparison is 368750cee35f31935640a34ad199e889fc60f1a8. Run `git diff 368750cee35f31935640a34ad199e889fc60f1a8` t

Codex·GPT-5.5·Soph·2mo ago·30hr 24min·8 Checkpoints·22 file changes·+374/-95·40.1K tokens

Review the code changes against the base branch 'main'. The merge base commit for this comparison is 368750cee35f31935640a34ad199e889fc60f1a8. Run git diff 368750cee35f31935640a34ad199e889fc60f1a8 to inspect the changes relative to main. Provide prioritized, actionable findings.

2mo ago·19.0s·4 calls

{"findings":[],"overall_correctness":"patch is correct","overall_explanation":"The change fixes the error path to report the parent hash already saved before the failed lookup, avoiding the previous nil dereference. The added regression test covers the missing-parent scenario, and planner tests pass.","overall_confidence_score":0.94}

653abf2Consume positional source/target left-to-right across commands sync/replicate/plan, bootstrap, and probe consumed positional args by fixed index (args[0]→source, args[1]→target), so `--source-url URL <target>` left the lone positional in args[0] and the target slot empty — sync/bootstrap/plan errored with "requires source and target"; probe failed silently, dropping the target and probing only the source. Extract the left-to-right consumption that convert-sha256 already did correctly into a shared resolvePositionalEndpoints helper, and route all four commands through it. When a positional is left over after both slots are filled (e.g. a flag-supplied source plus two positionals), reject it rather than silently ignoring one — the fixed-index code dropped a positional here too. Add a table test covering the mixed forms and the over-specified case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: b21841a5f646+85/-28
8bccb56Include the port in the git-credential host field credentialInput emitted host=<hostname>, dropping any non-default port. gitcredentials(7) defines host as "host[:port]", so on a non-default-port remote the fill query never matched the stored entry and approve/reject wrote to the default-port entry instead. Emit ep.Host (which keeps the port); the empty-host guard still uses Hostname(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: a384992be26e+20/-1
f6bc49fKeep a pack-observer Scanner error from aborting the upload The observer tees the pack stream through a goroutine running packfile.Scanner. When that Scanner stopped early (a malformed pack, or content it couldn't parse), its deferred pr.Close() broke the pipe, so the next TeeReader write failed with io.ErrClosedPipe and surfaced from Read — aborting the live push, despite the documented "non-fatal for the upload" contract. Tee through a bestEffortWriter that absorbs write failures and always reports success, so a stopped observer can never turn into a failed upload. The bytes keep flowing to the server and the Scanner error remains available via ScannerError() for debugging. (This removes the spurious push failure, so there is no longer an unclassified error for executeBatched to mishandle.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: b7dda726e331+52/-1
74a2fd5Treat a pack-less v2 fetch response as an error, not success consumeV2FetchPack returned nil when the response ended (flush or EOF) without ever delivering a packfile section, so a truncated or empty response let a fetch report success having stored nothing. By the time control reaches here wants is non-empty (fetchToStoreV2/fetchPackV2 short-circuit the up-to-date case earlier), so the server owes us a packfile. Return io.ErrUnexpectedEOF on the no-packfile flush/EOF paths, matching the sibling openV2PackStream. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: a96014b75712+31/-4
f81cf1cRelease session resources on partial-setup and error paths Two leaks in the syncer session lifecycle: - newSession opened the source transport (and started the memory-measurement ticker goroutine) before listing refs and setting up the target. If any of those steps failed it returned nil, err, so the caller had no session to finish() — leaking the source conn (an SSH transport spawns a process), a half-open target conn, and the ticker goroutine. - finish() never stopped the measurement ticker; it was only stopped when a Result was built, so any early error return from runSync et al. leaked the goroutine even on a fully-constructed session. Add a success-guarded deferred cleanup in newSession that calls finish() unless the session is handed back, hand the target conn to the session as soon as it is opened so cleanup can close it, and have finish() also invoke the sync.Once-guarded measurementDone so the ticker stops on every path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: e6526831fa74+76/-13
db22241git-sync-bench: exit non-zero on failed runs, drop -1 sentinels Two reporting bugs: - run() returned nil after printing the report regardless of outcome, so the process exited 0 even when every benchmark run failed — a failure could pass for success in CI. Return an error (after printing the report) when any run failed. - The aggregate Min* fields start at -1 as an "unset" sentinel updated by the first qualifying run. With no successful (or no batched) run the sentinel survived into the report, emitting minWallMillis=-1 etc. Clear the sentinels to 0 when there were no qualifying runs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: fde63c6432fc+43/-2
6b45fb8go.mod and test hygiene Four small hygiene fixes flagged in review: - go.mod pinned `go 1.26.2` in the go directive, forcing that exact patch toolchain on everyone. Use `go 1.26` for the language requirement plus a `toolchain go1.26.2` line. - ExampleClient_Sync ran with no timeout (the review flagged real network I/O during go test). The repo's golangci-lint config enforces testableexamples, so the example must keep its `// Output:` directive and remain runnable; instead of dropping it, bound the call with a 30s context deadline so it can't hang. The example hosts are RFC 2606 reserved names that don't resolve, so it fails fast and prints nothing. - The smart-HTTP test server's handlers called s.tb.Fatalf from the server's goroutines, where Fatalf's runtime.Goexit doesn't stop the test correctly. Use s.tb.Errorf (safe from any goroutine); each call is the handler's final statement, so the handler still returns. - SyncRequest.Validate and PlanRequest.Validate were byte-for-byte duplicates; extract validateSyncFields and have both delegate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 3456387d7a8a+28/-32
0eb86b0Use a real Windows lock for the file token store The initial Windows shim made the package compile but its lock was a no-op, so two concurrent file-store writers could lose an update or collide on the shared ".tmp" file. Use LockFileEx/UnlockFileEx (golang.org/x/sys/windows, already a dependency) on the ".lock" file for the same blocking, interprocess advisory exclusion the Unix path gets from flock, which serializes writeFileToken's read-modify-write. Promotes golang.org/x/sys to a direct dependency. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: aef01c5e047d+39/-14