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.

API Error: The socket connection was closed unexpectedly. For more information, pass verbose: true in the second argument to fetch()

Here's my full assessment of PR #66 (convert-sha256).

TL;DR

The core is production-quality and empirically correct: I converted spf13/cobra (37 refs, ~5.4k objects) and octocat/Hello-World end-to-end — git fsck --full clean, extensions.objectformat = sha256, origin-notes resolve, mapping TSV well-formed, and a --sign-mode tips attestation tag passes git verify-tag ("Good git signature"). Full test suite passes including -race and all gated git-http-backend + signing e2e tests. All actionable review comments are addressed.

I found one high-impact robustness bug, one doc inaccuracy, and a credential-leak edge worth fixing before merge. None affect the happy-path conversion.

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:

Verified non-issues (the workflow tried hard to break these and couldn't): core object encoding, discovery completeness + cancellation, ref-scope/pull-ref policy, --sign-mode validation + shell-safety, ticker concurrency. The inProgress cycle guard and a "two SHA1s → same SHA256 → duplicate notes entry" scenario are both cryptographically infeasible — one review agent spun out brute-forcing the latter, which is itself good evidence it can't happen.

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.

Minor test-coverage gaps (low, non-blocking — no bug, just confidence): mergetag stripping, SignatureSHA256 dual-hash stripping, lightweight tags (functionally validated by my cobra run), and the reproducibility guarantee are not unit-tested.

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.

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).

Done — fixes applied and verified

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.

The changes are uncommitted on nodo/convert-sha256. Want me to commit them (and/or reply on the pjbgf thread)?

I've traced it fully. Here's the current state.

Where this comment lives

That text isn't an open thread on the PR right now — it's the rationale that already drove commit 49ecbacd ("exclude foreign pull/merge-request refs from --all-refs"). The commit message mirrors your comment almost verbatim ("foreign to the repository until merged… git push --mirror… a destination forge… could republish unreviewed code as repo content"). So this concern was raised and actioned; there's no unresolved PR thread for it.

Current behavior — concern is addressed, secure-by-default

Pull/MR refs (RefKindOther) can only enter scope under --all-refs (planner: case kind == RefKindOther && cfg.AllRefs). The convert layer then layers a default denylist on top:

So the attack path you describe (bring in foreign PR code → push --mirror → forge with different refs/pull semantics surfaces it as repo content) cannot happen on any default path; it requires a deliberate flag the docs warn against. This is covered by tests: TestRun_GitHTTPBackend_PullRefs, TestEffectiveExcludePrefixes, TestCountForeignPullRefs, TestForeignPullRefPrefixes_NotProtected, and TestProtectedExcludePrefixes (you also can't accidentally drop branches/tags).

The one real gap worth your attention

The denylist is a fixed allowlist of three forges' namespaces:

Gerrit's refs/changes/* — unmerged changesets, exactly the same class of "foreign code before merge" — is not in the list. So a --all-refs conversion of a Gerrit-hosted source would convert refs/changes/* by default and is then subject to the identical republish risk. Same for any forge using a namespace outside these three. The secure-by-default guarantee holds precisely for the three named forges, not "any PR-like namespace."

That's a denylist-vs-broad---all-refs tradeoff, not a bug — but if your threat model is "any unmerged-proposal ref," it's a hole. Two reasonable responses:

1. Add refs/changes/ (Gerrit) to foreignPullRefPrefixes (cheap, covers the next-most-common forge), and mention the residual denylist limitation in the docs.
2. Leave as-is and document that --all-refs only auto-excludes the three named forges' PR namespaces — anything else needs an explicit --exclude-ref-prefix.

Want me to do #1 (extend the denylist + a test + doc note)? It folds cleanly into the changes already staged on this branch.

No — the default converts every branch and every tag, not just main. It's a deliberately fixed scope, not a single-branch default.

What the default (no flags) includes:

• All refs/heads/* (every branch)
• All refs/tags/* (every tag, lightweight and annotated)
• The tool's own side outputs: refs/notes/sha1-origin (unless --no-origin-notes)

You saw this in the live spf13/cobra run earlier — with zero scope flags it converted 8 heads + 37 tags, not just main.

What the default excludes (only --all-refs pulls these in):

• refs/pull/*, refs/merge-requests/*, etc. (and even under --all-refs these stay excluded unless --include-pull-refs)
• refs/notes/* from the source, refs/replace/*, and any other custom refs/* namespace

main only enters the picture for HEAD: after converting all branches, HEAD is pointed at the source's advertised HEAD (usually main), falling back to main/master/first-branch. So main is just where HEAD lands — every other branch is still converted alongside it.

This is intentional and can't be narrowed — there are no --branch/--tags flags. The reasoning (from the code/docs): a partial scope risks stranding cross-branch hash references in commit messages, which is the exact thing the message-rewrite pass exists to keep intact. So the spectrum is:

So relative to the pull-ref discussion: the foreign PR code is excluded at both the default and --all-refs levels — the default is actually the broad "all branches + all tags," and pull refs sit one explicit opt-in beyond even --all-refs.

👍 Recap of where things stand:

• Fixes applied & verified (uncommitted on nodo/convert-sha256): cleanup-on-failure, credential-leak, regression test, 3 doc corrections. Full suite green, lint clean.
• Untouched / your call: commit the changes; the Gerrit refs/changes/* denylist gap; replying on pjbgf's signing-tags thread; the deferred test-coverage gaps.

Ping me when you want any of those.

Done — committed and pushed.

• Commit: d755e2cd → origin/nodo/convert-sha256 (PR #66), fast-forward from 49ecbacd.
• Contents: cleanup-on-failure fix + credential-redaction fix in sha256convert.go, the gated regression test, and the three doc corrections.
• Remote tip confirmed at d755e2cd.

Still open if you want them later: the Gerrit refs/changes/* denylist gap, a reply on pjbgf's signing-tags thread, and the deferred unit-test gaps.