## 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.RemoveAll`s 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 _raw_`req.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

- ✅ cursor[bot] ctx-cancel → fixed (iterative DFS checks `ctx.Err()`)  
- ✅ pjbgf go-git alpha.4 SHA1 note → fixed (e7e4a1a, docs updated)  
- ✅ pjbgf `--sign` → `--sign-mode {none,tips}` → fixed (e7e4a1a)  
- ⏳ pjbgf "concerns on signing tags in place?" — **unresolved**, but nodo answered with a correct explanation and pinged the reviewer. No code change pending; it just needs pjbgf's ack.

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

- **Med:** §Operational Notes (lines 254–257) claims the notes commit "carries `time.Now()`" and is "the only thing that differs" across runs. The code pins it (`SOURCE_DATE_EPOCH`/Unix epoch) — I verified the notes ref is identical across runs. Stale, and it undersells a real selling point. **(Flip side of #1: the docs at 162–167 already promise the repo survives a signing failure — so the code is the bug, not the docs there.)**  
- **Low:**`SOURCE_DATE_EPOCH` support is implemented but undocumented.  
- **Nit:** docs say HEAD points at "the source's symbolic HEAD"; the code actually has a `main`→`master`→lexicographically-first fallback worth a mention.

---

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)

- **PR thread:** pjbgf's "signing tags in place?" question is answered and awaiting their ack — no code change pending.  
- **Deferred test gaps** (low, not bugs): mergetag stripping, `SignatureSHA256` dual-hash stripping, lightweight tags, and the reproducibility guarantee aren't unit-tested. I left these out per the earlier plan; say the word and I'll add them.