Follow-ups from PR #46 review (did-git-sign)

[stormer78]

Follow-up items from the code review of #46. None are blockers for that PR; tracking them here so they aren't lost.

Issues / Suggestions

1. Dead condition in unrecognised-subcommand guard

In did-git-sign/src/main.rs (the None => { ... } arm), the guard reads:

if let Some(f) = &cli.sign_file {
    if cli.operation.is_none() {
        anyhow::bail!(...);
    }
}

The outer -Y handling returns early before this arm is reached, so cli.operation.is_none() is always true here. The inner check is dead — the outer if let Some(f) alone is sufficient. Minor, but the condition reads as defensive code for a branch that can't occur.

2. -Y verify args beyond -s/-I/-O could break again

Git's man page currently documents these flags, but any future flag (e.g. -U, -r) will hit the same clap rejection that fixes 6/7 solved. Consider #[arg(trailing_var_arg = true, allow_hyphen_values = true)] on a catch-all Vec<OsString> for the delegation path so new flags forward automatically — matching the "catch-all operation" split philosophy already applied one level up.

3. process::exit(code) after #[tokio::main] — stdio flushing note

The inline safety note is good, but stderr/stdout flushing behaviour is worth calling out explicitly: inherited stdio doesn't buffer in-process, so it's safe as-is. If future code adds any println! between delegation and exit, the Drop-skip becomes relevant. Consider adding that nuance to the existing comment.

4. DID_GIT_SIGN_SSH_KEYGEN trust boundary

An attacker with write access to the user's env can redirect verification to a fake binary that always returns 0. Given the tool already runs in the user's shell context this is not a new attack surface, but a one-line warning in the README (or rejecting the override when DID_GIT_SIGN_STRICT=1) would match defense-in-depth expectations for signing tooling.

5. user.signingKey set to a .json path

The init notice mitigates UX surprise, but some third-party git GUIs parse user.signingKey assuming it's a key path and may error/warn. Acceptable trade-off as documented — worth tracking if users report it.

6. setup_git_never_writes_user_email CWD invariant is implicit

The CwdGuard is dropped before the assertion and the verify command uses -C dir.path(), so the test is correct — but the invariant depends on a reader noticing the drop-point comment. Adding assert_eq!(std::env::current_dir().unwrap(), original) after the inner block would make the invariant explicit and catch future edits that move the assertion inside the guard's scope. Nit.

7. PR description / code mismatch on --email flag

The #46 description states the --email flag was removed, but no such field appears to be removed in the diff. Either the removal is missing or the description is inaccurate — worth confirming which, and correcting whichever is wrong.

Risks

R1. Leftover local user.email on upgrade

After upgrade, setup_git no longer writes user.email, but it also does not unset an existing DID value from a prior install. Users who previously ran init still have the broken email locally until they clear it manually. Options:

• (a) Actively git config --local --unset user.email in setup_git when the current value looks like a DID.
• (b) Document the cleanup step in the init output.

R2. No integration test exercising the full git commit → sign → log --show-signature loop

Unit tests cover each piece, but the seven bugs fixed in #46 all originated from the integration gap. A shell-based e2e test (even gated behind #[ignore]) would prevent recurrence.

---

Source: review of #46.

[stormer78]

Addressed in the v0.2.0 release branch as commit 4d8d72a (PR #58). Status of each item:

#	Item	Status
1	Dead inner cli.operation.is_none() condition	✅ Removed; comment explains why no inner check is needed
2	-Y verify clap catch-all for forward-compat	⏸ Deferred — larger reshuffle; tracking for v0.3
3	process::exit stdio-flushing comment	✅ Expanded with the inherited-stdio rationale + the exact tripwire that would change it
4	DID_GIT_SIGN_SSH_KEYGEN README warning	✅ Added under Security Model — marks the env var as test-only and explains the verification-bypass risk
5	user.signingKey .json path third-party-tool tracking	📝 Watch-item — no action unless users report it
6	setup_git_never_writes_user_email CWD invariant	✅ Pinned with explicit assert_eq!(current_dir(), original_cwd)
7	PR #46 --email flag mismatch	✅ Resolved by correcting the stale did_key_id doc-comment in config.rs (the comment claimed it was used as user.email, which has been false since #46)
R1	Active unset of stale DID-shaped user.email on upgrade	⏸ Deferred to v0.3
R2	Full-loop git commit → sign → log --show-signature integration test	⏸ Deferred — substantial fixture work; tracking for v0.3

Items 1, 3, 4, 6, 7 land here. 2, 5, R1, R2 are tracked for the v0.3 cycle.

Thanks for the thorough review @stormer78 — closing in favour of #58.