Preserve explicit grove clone URLs

[mfreeman451]

Summary

• preserve explicit grove clone URL overrides instead of normalizing every value to HTTPS
• keep schemeless remote labels normalized for consistency
• add focused coverage for local paths and explicit http/ssh overrides

Testing

• go test ./pkg/hub -run 'TestNormalizeCloneURLLabel|Test.*Clone.*URL|Test.*Grove.*Create.*Clone'\n

[mfreeman451]

lgtm

[ptone]

PR #117 Review: Preserve explicit grove clone URLs

Executive Summary

This is a low-risk refactoring that extracts clone URL resolution into a dedicated, well-tested module. The change fixes a real bug in populateAgentConfig where explicit non-HTTPS clone URLs (e.g., http://, ssh://, git@) were silently rewritten to https://, and also fixes a latent double-scheme bug in the same function's fallback path.

Files Changed

File	+/-	Description
pkg/hub/clone_url.go	+40	New file: normalizeCloneURLLabel and resolveCloneURL
pkg/hub/clone_url_test.go	+90	New file: unit tests for both functions
pkg/hub/handlers.go	+2/-15	Replace inline logic at two call sites

Critical Issues

None.

Observations

1. file:// URLs fall through to HTTPS normalization (Minor)

normalizeCloneURLLabel recognizes http://, https://, ssh://, git://, and git@ prefixes, but not file://. A file:// URL would fall through to util.ToHTTPSCloneURL(), which would strip the scheme and mangle the URL. While file:// is rare in production, it's a plausible local-testing scenario similar to the absolute-path case already handled.

Suggested Fix:

for _, prefix := range []string{"http://", "https://", "ssh://", "git://", "file://"} {

2. Behavioral parity in populateAgentConfig fallback (Positive change, worth noting)

The old code in populateAgentConfig had:

cloneURL = "https://" + grove.GitRemote + ".git"

If grove.GitRemote already contained a scheme (e.g., https://github.com/org/repo), this would produce the malformed https://https://github.com/org/repo.git. The new code correctly routes through normalizeCloneURLLabel, which preserves scheme-prefixed remotes and only applies HTTPS conversion to schemeless values. This is a latent bug fix.

3. Test coverage is good but could add one more edge case

Consider adding a test for an https:// URL that already has .git suffix vs. one that doesn't, to verify ToHTTPSCloneURL isn't double-appending .git. This is tested in the util package presumably, but a regression test in the new module would be defensive.

4. filepath.IsAbs is platform-dependent (Non-issue)

filepath.IsAbs uses OS-specific logic, but since this is a server-side Go binary running on Linux, this is fine. Windows paths (C:\...) are irrelevant here.

Positive Feedback

• Clean extraction: Moving duplicated URL resolution logic into a single, tested function is textbook refactoring. Two call sites with slightly different (and subtly buggy) inline logic are replaced with one correct implementation.
• Well-structured tests: Both functions have table-driven parallel tests covering the key cases: local paths, schemeless remotes, explicit HTTP, and SSH shorthand.
• Correct case handling: Using strings.ToLower for prefix matching while returning the original cloneURL preserves user-specified casing.
• Clear function contract: resolveCloneURL(override, gitRemote) makes the precedence obvious - override wins, fallback to remote, both normalized.

Final Verdict

Approve. The file:// observation is minor and non-blocking. The change is correct, improves maintainability, fixes a latent bug, and has solid test coverage.