Fix Enable Report Timeout and URL Stripping

Confirmed: ReportEnable's response is discarded everywhere in production code. Here's my review.

---

Review: cli-report-entire-enable

Reports entire enable to the backend so web onboarding reflects the enabled repo. Small, well-scoped change (3 commits, +197 lines). The design is sound — strictly best-effort, fully silent, and the security-conscious commits ("redact remote creds") show good instinct. The API client follows existing package conventions exactly, and the credential-stripping is correct: cleanURL is rebuilt from parsed Host/Owner/Repo components rather than the raw remote, so embedded https://token@host/... credentials and query params can never leak. Tests for the API client are solid.

A few things worth addressing before merge.

Findings

1. No timeout on the report — entire enable can hang at the end (Medium)

reportRepoEnabled runs synchronously in a deferred call on the success path, using a Client whose http.Client has no Timeout and relies on http.DefaultTransport (which has no response-header timeout). If the backend accepts the connection but never responds, entire enable hangs after it has already printed success — with zero output explaining why, since the report is "fully silent." This is worse in --yes/CI runs.

Note the contrast with the established best-effort reporter, telemetry SendEvent, which explicitly runs in a detached subprocess (__send_analytics) precisely so it never blocks the command — its comment even says "no need for fast timeouts since we're detached." This new path is in-process, so it needs the timeout the detached one can skip:

This matches the codebase's pattern for non-critical network/git calls (status.go:464, hooks_cmd.go:46, etc.).

2. EnableRepoResponse fields are entirely unused in production (Low)

Connected, InstallURL, and Repo are decoded but only read in tests. The doc comments ("when it can't, InstallURL points at the App installation page") describe behavior the CLI never acts on — ReportEnable's caller discards the response with _. That's a defensible API-contract choice (the comment says the web onboarding surfaces the nudge), but as written the comments read as if the CLI uses these. Either trim the response to what's consumed, or add a one-line note that the fields exist for the contract/future use and the CLI deliberately ignores them today.

3. Comment/behavior mismatch in EnableRepoRequest (Nit)

The comment says "The server parses the raw remote URL itself, so the CLI only needs to send what it knows." But the CLI deliberately does not send the raw remote — it sends a reconstructed, credential-free, https://-normalized URL (correctly so). The "raw remote URL" phrasing undersells the redaction that's actually the point of commit a59465e3a. Worth tightening so a future reader doesn't "simplify" by sending the raw URL back.

Things I checked that are fine

• Defer LIFO ordering: Correct. The report defer is registered first (runs last), the bootstrap-finalize defer is registered later (runs first), so the report sees the origin remote created by finalize. And because finalize can set runErr, a finalize failure correctly suppresses the report.
• Non-GitHub / no-origin / not-logged-in paths: All return early and silently as documented.
• --insecure-http-auth flag: Consistent with every other authenticated command (hidden dev flag).
• Tests pass: go test ./cmd/entire/cli/api/ -run TestClient_ReportEnable ✅

Suggestion

reportRepoEnabled has no test, and its two security/robustness-relevant behaviors — credential stripping and the no-origin skip — are exactly the kind worth pinning. The function is awkward to unit-test as-is (git remote + auth client), but extracting the URL-cleaning into a tiny pure helper (rawURL → cleanURL, ok) would make the redaction directly testable and guard against regressions.

Want me to apply the timeout fix (#1) and tighten the comments (#2, #3)?

All three fixes applied, lint clean, build and tests pass.

Summary

1. Timeout (setup.go) — reportRepoEnabled now wraps ctx in a 5s context.WithTimeout, so a backend that accepts the connection but never responds can no longer hang entire enable after it has already printed success. Added the time import.

2. Unused response fields (enable.go) — Added a doc note on EnableRepoResponse stating the CLI deliberately ignores Connected/InstallURL/Repo (the nudge is surfaced by web onboarding), and that they're decoded for the API contract / future use.

3. Misleading request comment (enable.go) — Rewrote the EnableRepoRequest comment to say RemoteURL is a clean, credential-free URL (credentials/query params stripped before sending), instead of "the raw remote URL," so a future reader won't undo the redaction.

Verified: mise run fmt && mise run lint → 0 issues; go build ./... clean; api and cli package tests pass.

One thing I did not do (it was a suggestion, not in 1–3): extracting the URL-cleaning into a testable pure helper. Happy to do that as a follow-up if you want the redaction pinned by a unit test.

Done. Two commits on cli-report-entire-enable:

1. 4b3e93dda — the three review fixes: 5s timeout on the synchronous report, and clarified API doc comments (credential-free RemoteURL, deliberately-ignored response fields).
2. 5d7f827c7 — extracted cleanRemoteURLForReport as a pure helper and added TestCleanRemoteURLForReport, a table-driven test covering:
   • https token (ghp_...@) and user:password@ credentials stripped
   • query params dropped
   • scp-style git@github.com:... normalized to https with the user dropped
   • missing .git suffix added
   • unparseable single-segment path returns an error (caller skips reporting)
   • plus per-case guards asserting no known secret string survives in the output

mise run fmt && mise run lint is clean (caught and fixed a wrapcheck issue on the extracted helper's error path along the way), and the cli + api package tests pass.

The redaction behavior is now pinned — if anyone later "simplifies" by forwarding the raw remote, the leak guards fail. The one behavior I intentionally left as-is: the helper drops a non-default port (uses info.Host, not HostPort()). That's fine for the GitHub-App-centric use case here, but flag it if you ever need to support a self-hosted forge on a custom port.

Fixed and committed (02782a40c).

What was wrong

cleanRemoteURLForReport rebuilt the URL from info.Host. For a mirrored repo whose origin is entire://aws-us-east-2.entire.io/gh/owner/repo, ParseURL returns Host = aws-us-east-2.entire.io (the Entire cluster) with Forge = gh. So the helper produced https://aws-us-east-2.entire.io/owner/repo.git — the backend can't resolve that as a GitHub repo, and enable reporting silently failed for exactly the repos that are already onboarded via the mirror.

Fix

Added Info.CanonicalHost() to the gitremote package rather than hardcoding gh → github.com at the call site, so the forge↔host knowledge stays co-located with hostToForge:

• Derived forgeToHost as the reverse of the existing hostToForge map (single source of truth — adding a forge later updates both directions automatically).
• CanonicalHost() returns the forge's real host when the forge is known (gh → github.com, covering both direct github URLs and entire:// mirrors), and falls back to Host when the forge is unknown — so a self-hosted GitHub Enterprise remote (git@ghe.corp.example.com:..., Forge="") is preserved rather than mangled.

Tests

• TestInfo_CanonicalHost in the gitremote package: direct https/ssh github, entire:// mirror, and unknown-forge fallback.
• Extended TestCleanRemoteURLForReport with the entire:// mirror case (→ github.com) and the self-hosted-enterprise case (host preserved).

All green, mise run fmt && mise run lint clean. Three commits now on the branch (timeout/docs, helper extraction + tests, this host fix).