Review the current state of this change, test it on a repo, make sure all the PR comments are address, make an assessment of the docs. · Entire

Review of Current Change

Review the current state of this change, test it on a repo, make sure all the PR comments are addressed, make an assessment of the docs.

1. Current State of the Change — Reviewed

I read the full 1,803-line core plus the CLI layer, and ran a 7-dimension adversarial review workflow. The two-pass design (discovery → memoized topological translation) is correct: tree/commit/tag re-encoding preserves modes/ordering/headers, strips gpgsig/gpgsig-sha256/mergetag, message-reference edges are added to the DFS so cross-branch refs resolve, and loose objects go through SetEncodedObject (correct on the pinned go-git alpha.4 pseudo-version). Discovery checks ctx.Err() per object; signBranchTips uses exec with an argv slice (no shell injection); the --progress ticker is correctly synchronized with atomics + a close/done handshake.

Confirmed findings:

# Sev Finding
1 High Failure in an optional post-step deletes the complete conversion. After refs+HEAD are written the repo is valid, but cleanupTarget stays armed through notes / --write-mapping / --sign-mode tips. Any error there os.RemoveAlls the whole target (unless --keep-source-objects). I reproduced this twice: an unwritable --write-mapping path and a missing signing key both wiped a fully-converted repo. This contradicts the code's own comment (lines 466–470, "surface partial signed tags so the caller can clean up") and the docs (see #2). Fix:cleanupTarget = false right after HEAD is set (~line 430) — same logic the --check path already applies. For a kernel-scale run this is hours of work lost to a path typo.
2 Low‑Med Credential leak on an unparseable URL.redactSourceURL safely placeholder-redacts, but openSource (line 890) re-parses the rawreq.SourceURL and url.Parse's error embeds the full URL — so a token-bearing URL that fails to parse leaks the token into the error (→ logs/CI). A real hole in the redaction work (commit ffda0901). Fix: don't echo the raw URL in that error.

2. Tested on a Repo — Done

Unit + -race: pass. Gated e2e (incl. signing, pull-refs, side-output collision): 24/24 pass (GO_TEST_EXIT=0). Live spf13/cobra: 4s, fsck-clean, signature verifies. Plus I confirmed determinism: two runs produced identical branch/tag and notes-ref hashes.

3. PR Comments — All Addressed

4. Docs Assessment — Excellent, with 2 Fixes

docs/convert-sha256.md is unusually thorough and accurate on the hard parts (signature stripping, submodule fail-fast, foreign pull refs, message-rewrite robustness, the alpha.3→alpha.4 history). Issues:

Want me to apply the fixes for #1 (cleanup-on-failure) and the two doc corrections? Those are clear and low-risk. The credential-leak fix (#2) I'd also bundle in. The test gaps I'd leave to a follow-up unless you want them now.

Final Summary

Everything's green: go test (race + gated e2e) exits 0 for both packages, gofmt clean, golangci-lint reports 0 issues, and the regression test is proven to bite (passes with the fix, fails without).

Code (cmd/git-sync/internal/sha256convert/sha256convert.go)

  1. Cleanup-on-failure (the High finding):cleanupTarget is now disarmed the moment the conversion is complete (after refs + HEAD). A failure in origin-notes, --write-mapping, --sign-mode tips, or --check now surfaces the error but leaves the valid converted repo on disk — instead of silently deleting it. Removed the now-redundant disarm in the --check path and tidied the success-path comment.
  2. Credential leak:openSource no longer propagates url.Parse's error verbatim (it embeds the raw URL); it surfaces only the underlying *url.Error.Err reason, so a token in a malformed source URL can't leak into output/logs.

Test (..._test.go) 3. Added TestRun_GitHTTPBackend_KeepsTargetOnPostConversionFailure — gated e2e that forces a mapping-write failure and asserts the converted repo survives + stays fsck-clean. I confirmed it fails without the fix ("converted target was deleted") and passes with it.

Docs (docs/convert-sha256.md) 4. Corrected the stale determinism claim — the notes ref uses a pinned timestamp (SOURCE_DATE_EPOCH/Unix-epoch), not time.Now(), so the whole conversion (including notes) is reproducible; this also documents SOURCE_DATE_EPOCH.
5. HEAD selection now reflects the real fallback (advertised → main/master → first branch).
6. --check refs description now notes signed attestation tags are also excluded side outputs.

Verification: gofmt clean · go vet clean · golangci-lint 0 issues · full suite with -race + gated git-http-backend/signing e2e all pass · all three failure scenarios empirically re-confirmed (target kept + fsck-clean; token no longer leaked).

Still Open (Your Call)