Entire

I’m reviewing the current branch against its base and checking the PR description so I can look for behavioral regressions, missing tests, and mismatches between the code and the stated intent.

I have the PR intent. Next I’m checking the actual diff and the touched tests to see whether the implementation matches that description and whether anything looks unsafe or incomplete.

The tests cover the claimed scenarios. I’m reading the surrounding planner and sync path now to see whether the new eligibility rules line up with how relay actually builds packs and whether the HTTP error formatting has any edge cases.

The relay path does appear to rely on the same no-thin invariant already documented in fetch.go, so I’m verifying the branch-create and materialized cases with focused tests before I decide whether there are review findings.

I’m down to the edge cases now. The only concrete gap I’ve hit is test execution in internal/syncer: the sandbox blocks httptest from binding a local port, so I can’t fully verify that package here without escalation.

No findings.

The PR description matches the code changes I reviewed. The three claimed behaviors are reflected in the implementation: ObjectsToPush now tolerates pruned top-level and transitive objects in planner.go, incremental relay now accepts branch creates and no-thin targets in relay.go, and httpError now includes diagnostic headers in smarthttp.go. The added unit/integration coverage in planner_test.go and integration_test.go lines up with the PR description.

Verification: go test ./internal/planner and go test ./internal/gitproto passed. I could not fully run ./internal/syncer in this sandbox because one existing test uses httptest.NewTLSServer and local port binding is blocked here, so there is still a small unverified gap around the integration package.

I’m reviewing the branch diff against main and checking the touched code paths for regressions, unsafe assumptions, and test gaps.

The branch appears to be at the same commit range as origin/main, so I’m checking the working tree for uncommitted changes and reviewing those directly.

This is a CLI refactor from flag to cobra. I’m checking each subcommand wrapper for parity with the old argument/flag behavior and whether the new error handling changes exit semantics.

I’ve narrowed the review to CLI behavior changes. The refactor is structurally fine, but I’m checking for contract regressions against the old entrypoint semantics, since that’s where user-visible breakage is most likely.

Medium: bare git-sync now exits successfully instead of failing for missing command. In the old entrypoint, run() returned a usage error when len(args) == 0, so the process exited with status 1. After the Cobra refactor, the root command’s RunE just calls cmd.Help() and returns nil, and main() treats that as success. That is a user-visible contract change for scripts that validate invocation or expect misuse to fail. See root.go and main.go.
Low: unknown subcommand flags now print the root usage block instead of the subcommand usage, which makes the new error-handling path materially less helpful than intended. run() always calls showUsage(cmd, err) on the root command object after ExecuteContext fails, so git-sync sync --bogus shows top-level usage rather than sync’s flags. I verified that behavior locally. See main.go and main.go.

Residual risk: this is a large CLI surface refactor with no cmd/git-sync tests added, so argument parsing and exit-code parity are mostly unguarded right now. I spot-checked go run ./cmd/git-sync, go run ./cmd/git-sync --version, go run ./cmd/git-sync version, and go run ./cmd/git-sync sync --bogus; the second finding came from that check.