Entire

Reviewed the branch. The fix is correct and well-targeted. Here's my assessment, including the cobra question.

Verdict: solid, ship it

The root cause is real: pflag captures DefValue from flag.Value.String() at registration time, and FlagUsages prints (default "…") for any non-empty default — both in --help and in the usage block dumped on a parse error. Seeding the env value as the flag default therefore leaks it (into CI logs, etc.). The fix — register secret flags with an empty default and apply the env value post-parse in PreRunE, gated on Changed() so an explicit flag wins — is the right shape. Tests pass and cover both the leak and the fallback/precedence behavior.

The chainPreRunE extraction is necessary, not incidental: previously allRefsFlag replaced PreRunE, so now that multiple helpers (allRefsFlag + several addSecretFlag) attach to the same command, clobbering would be a real bug. They all chain correctly, and their effects are independent (bools vs. distinct string targets), so ordering is safe.

I also verified the parent/child PreRunE trap doesn't apply here: cobra only runs the executed leaf command's PreRunE (not parents'). All auth flags attach to leaf commands (sync, replicate, plan, bootstrap, probe, fetch, convert), each with its own RunE, so the env fallback always fires. If anyone ever moves these onto a parent command with subcommands, the fallback would silently stop running — worth a mental note but not actionable now.

Your question: does cobra have better built-in handling?

Not really — not without changing dependencies:

Cobra has no native env-var binding. The "blessed" built-in route is viper (viper.BindPFlag + BindEnv/AutomaticEnv). With viper the flag default stays empty and you read values via viper.GetString, so the leak wouldn't arise. But it's a heavier dependency and a different access pattern (read-from-viper instead of bind-to-struct-field), and this repo doesn't use viper anywhere. Adding it just for this would be a net negative.
Within pflag, the only shorter lever is registering with the env default and then clearing the displayed default: cmd.Flags().Lookup(name).DefValue = "". It suppresses the usage print while keeping the value, so no PreRunE needed. But it's hackier (mutating an internal string), and it loses the explicit-flag-vs-env distinction that Changed() gives you here. The current approach is cleaner and more correct.
MarkHidden would hide the flag from usage entirely — not what you want, since the flags should stay documented.

So: the chosen pattern is the idiomatic non-viper solution. No built-in does it better.

Minor notes (optional)

convert_sha256.go still wires the source flags inline rather than calling addSourceAuth. The PR correctly fixed the inline copy, but the duplication is exactly how the leak could creep back in. The new addSecretFlag comment already warns against reaching for envOr; if convert could reuse addSourceAuth, you'd remove the second place to get it wrong. Pre-existing, not introduced here.
addSecretEnvFallback has a single caller (addSecretFlag). Splitting is fine for naming/readability, but it could be inlined. Trivial.
--source-username etc. still print their (default "git") — correct, not secret.

Want me to push a small follow-up collapsing convert_sha256.go's inline source flags onto addSourceAuth, or leave it?

I've got PR #79 and the one inline review comment from pjbgf. Here's my take on both.

The PR itself: well-constructed

The core idea is right. BuildPlans runs the ancestry check against a store deliberately pruned of everything reachable from the target's refs, so a missing object means "the target already has this," not "error." Making the walk frontier-aware and returning a three-valued result is the correct altitude — fixing it at the fetch layer (one closure per ref) would, as the PR body argues, defeat the single-closure bandwidth optimization.

Logic checks out on the details:

Reachability wins over the frontier — the walk returns AncestryReachable the moment it sees targetHash, before any frontier consideration. So a provable FF is never downgraded to indeterminate.
hitFrontier is only consulted at the end, after the full reachable sub-graph is exhausted, so Unreachable (true divergence with a visible merge base) and Indeterminate (deciding commits pruned) are cleanly separated.
Tests cover all three outcomes plus the PlanRef block-vs-force behavior. The missing-start and pruned-ancestor cases are exactly the two failure modes from the problem statement.

The reviewer's comment: correct, worth taking

pjbgf is pointing at a real defensive-design gap. Right now:

and every error path returns the literal 0:

So 0 on the error path is the same value as AncestryReachable. Any caller that ever does result, _ := CheckAncestry(...) and ignores the error would read a depth-exceeded or load failure as "provable fast-forward" → ActionUpdate → potentially clobbering a ref. The zero value is the most dangerous possible outcome.

It's not an active bug — the only caller, PlanRef, checks err before looking at the result. So this is defense-in-depth, but it's cheap and the failure mode it guards against (silent overwrite) is severe enough that I'd take it.

I'd go slightly further than the suggestion: adopt AncestryInvalid as the iota-zero sentinel and replace the literal return 0, … with return AncestryInvalid, … so the intent reads at the return site rather than relying on 0 happening to line up:

This is safe to change: nothing relies on AncestryReachable == 0 (the benchmark and tests all compare against named constants), so no behavior shifts.

Want me to apply that — update the const block, switch the three return 0, … sites to return AncestryInvalid, …, and re-run go test ./internal/planner/?

Done. Applied pjbgf's suggestion plus the return-site cleanup:

Added AncestryInvalid as the iota-zero sentinel, with a comment explaining why the zero value must not be AncestryReachable.
Switched all three error returns (load source commit, ErrAncestryDepthExceeded, load parent commit) from the literal 0 to AncestryInvalid, so the intent is explicit at each return.

go test ./internal/planner/ passes — nothing relied on AncestryReachable == 0, so no behavior changed; the zero/error path is now just safe by construction.

Want me to commit this (and reply to pjbgf's comment resolving it), or leave it staged for you?