Entire

I’ll review the current branch in place first: branch/PR metadata, diff against its base, then focused file reads and any targeted tests that look necessary.

The branch is soph/shallow-metadata-disconnect-false-positive, one commit ahead of origin/main. I’m checking the PR metadata now so I can review against the intended base rather than guessing.

gh doesn’t find a PR for the local branch name, but the commit is also on origin/fix-shallow-merge-base-false-positive-in-metadata-disconnection-check. I’m going to inspect that branch/PR and diff from origin/main.

The PR is #1434 against main, and it touches metadata reconciliation plus a new shared git helper and remote checkpoint fetch behavior. I’m reading those diffs and nearby call sites for behavior changes.

A key shape here: the PR changes the strategy path to suppress “disconnected” results in shallow repos, while doctor tries to deepen first for an authoritative check. I’m checking whether that distinction holds in call sites and tests.

The tests cover the core false positive and genuine full-history disconnection cases. I’m now looking for edge cases around shallow detection scope, fetch refspec behavior, and whether the new helper changes global repository shallow state unexpectedly.

One possible concern is whether --unshallow can be scoped to only the metadata branch. Git’s shallow state is repository-wide, so I’m checking the existing fetch wrapper and tests to see if the PR’s behavior matches the intended tradeoff.

I’m checking the push path too because reconciliation runs there, not just under doctor. If the PR only fixes doctor but leaves pre-push able to hit the same false positive, that would be a real regression gap.

The implementation has one area I’m going to verify by tests: changed packages plus the CLI package because doctor is in cli. This should catch compile/import issues and the new shallow cases.

The targeted test command hit a sandbox restriction in a remote package test that opens an httptest listener (bind: operation not permitted). I’ll let the rest of the command finish, then rerun targeted tests with the approved escalation path if needed.

The targeted tests pass outside the sandbox. I’m doing a final pass for logic issues that tests may not catch, especially around the doctor command’s user-facing output and push-time reconciliation.

CI currently has test-core failing while lint and canary pass. I’m pulling the failing job log now; if it is related to this branch, it should be part of the review.

The failed CI job’s logs are not available yet because the workflow is still in progress, so I can’t attribute that failure from CI output right now. I’ll continue reviewing locally.

There’s a likely review issue in the pre-push path: reconciliation now suppresses a shallow merge-base miss, but the next step still calls plain git merge-base. I’m checking whether existing tests cover that path or whether it remains untested.

Another edge case is repository-level shallow state: git rev-parse --is-shallow-repository is not scoped to the metadata refs. I’m doing a small local Git reproduction to see whether --unshallow against the metadata ref clears unrelated shallow boundaries.

The local reproduction shows --unshallow clears the repository’s shallow state, not just the metadata ref. I’m checking whether that should be raised as a finding or just a comment/documentation mismatch based on surrounding intent.

The workflow has finished and the aggregate test plus test-core jobs failed, while lint, canary, and integration passed. I’m pulling the failure details now.

CI failure is from the two new strategy tests: they run git commit with isolated git env but never configure repo-local author identity, so GitHub Actions rejects the commits. That is a concrete review finding.

I have enough for the review: one definite CI-breaking issue in the new tests, and one behavior risk around doctor unshallowing whole shallow clones. I’m doing a final status check before I summarize.

Findings

P1: metadata_reconcile_test.go initializes repos with raw git init, then commits while using testutil.GitIsolatedEnv() without repo-local user.name / user.email. CI fails exactly here with “Author identity unknown” for both new tests. Use testutil.InitRepo(t, dir) or configure identity after git init.
P2: doctor.go deepens whenever the repo is shallow, and git_operations.go does that with --unshallow. That is repository-global, not metadata-ref scoped, so entire doctor can convert a shallow source checkout into a full clone just to check metadata. I’d gate this on the metadata refs actually reaching a shallow boundary, or make the potentially large fetch explicit.

Verification

env GOCACHE=/tmp/go-build go test ./cmd/entire/cli/checkpoint/remote ./cmd/entire/cli/strategy ./cmd/entire/cli passes outside the sandbox.
git diff --check origin/main...HEAD passes.
PR #1434 CI: test-core fails from the new test author identity issue; lint, canary, and integration pass.