Fall back to effective URL for forge detection with url.insteadOf

[amodelaweb]

When users configure url.insteadOf for multi-key SSH setups (e.g., git@github-work:org/repo.git rewritten to git@github.com:org/repo.git), the raw remote URL has a custom hostname that doesn't match any known forge. This breaks CI status, PR detection, and push-remote features.

The fix: add effective_remote_url which uses git remote get-url (applies insteadOf rewrites) and cache results per-remote in RepoCache. This is a local operation (no network I/O). When no insteadOf rules are configured, the effective URL equals the raw URL — no behavioral change. Both remote_url (raw) and effective_remote_url (rewritten) are needed because insteadOf can be configured in either orientation relative to forge URLs — forward (custom hostname → real forge, e.g. multi-key SSH) or reverse (real forge → internal destination, e.g. corporate mirrors).

Key changes:

• effective_remote_url cached per-remote via DashMap — free after first call
• Forge detection callers (find_forge_remote, platform_for_repo, github_push_url, detect_github, fetch_pr_info) use effective_remote_url
• find_remote_for_repo checks both raw and effective URLs (needs both for forward and reverse insteadOf scenarios)
• Removed forge_remote_url, is_known_forge, primary_forge_remote_url — superseded by the cached effective_remote_url
• Fixed detect_github using raw remote_url for remote branch owner lookup

This was written by Claude Code on behalf of amodelaweb

[worktrunk-bot]

CI is failing due to 6 collapsible_if clippy lints — nested if statements that can be collapsed using let chains. The code itself is correct; just needs the nesting flattened.

Locations:

• src/git/repository/remotes.rs:98, :105, :106, :107 — in forge_remote_url()
• src/git/repository/branch.rs:188, :189 — in github_push_url()

For example in forge_remote_url(), the three nested ifs at lines 105-117 can become:

if let Some(effective_url) = self.effective_remote_url(remote)
    && effective_url != raw_url
    && let Some(parsed) = GitRemoteUrl::parse(&effective_url)
    && parsed.is_known_forge()
{
    log::debug!(...);
    return Some(effective_url);
}

Same pattern applies to the other locations. There's also a formatting lint from pre-commit (collapsing a multi-line run_git call in the test).

codecov/patch failed — uncovered fallback paths in consumer code

[worktrunk-bot]

codecov/patch is failing — the core forge_remote_url() function is well-tested, but the consumer-side fallback paths that call it are uncovered:

• github_owner_repo() fallback loop (src/commands/list/ci_status/github.rs:32-42) — only runs when all raw URLs are non-GitHub and an insteadOf rewrite resolves to GitHub
• platform_for_repo() fallback loop (src/commands/list/ci_status/platform.rs:173-187) — same pattern for platform detection
• github_push_url() fallback (src/git/repository/branch.rs:187-195) — requires custom push remote with insteadOf
• primary_forge_remote_url() (src/git/repository/remotes.rs:214-218) — only called from fetch_pr_info() which isn't integration tested

These are tricky to test since the consumers also need gh CLI or push remote configs. A few options:

1. Unit tests with a helper repo that has custom hostname + insteadOf configured, calling github_owner_repo() and platform_for_repo() directly (they're pub / module-visible)
2. Extract and test the fallback iteration — the two-pass "raw URLs then forge_remote_url fallback" pattern is duplicated across github_owner_repo and platform_for_repo. A shared helper like find_forge_remote_url(repo, |parsed| parsed.is_github()) would be testable independently and reduce duplication
3. Accept partial coverage if the consumer paths are considered sufficiently covered by the forge_remote_url unit tests (the consumer logic is just iteration + delegation)

[max-sixty]

is setting the type of forge in the configs an alternative?

does this require an additional network request, and if so, when?

@amodelaweb if we did go ahead, we would need to get the test coverage much higher, potentially by consolidating some of the code

[worktrunk-bot]

Good questions — answering each:

Config alternative: There's already [ci] platform = "github" in project config for CI status detection, but forge detection is also used by wt switch pr:123 (for gh --hostname), github_owner_repo() (for API paths), and github_push_url(). A broader forge = "github" config could cover all of these, but it would mean users need to configure something worktrunk could infer automatically — git already has the information via insteadOf, so reading it feels more ergonomic. That said, an explicit config would be simpler and more predictable. Happy to go either direction.

Network requests: None. git remote get-url is purely local — it reads .git/config and applies url.insteadOf rewrites in-process without any network I/O. The only cost is spawning an extra git subprocess on the fallback path (when the raw URL hostname isn't recognized).

Test coverage: Agreed — the codecov failures are real gaps. Consolidating the two-pass "raw URLs then effective URL fallback" iteration into find_forge_remote() (which this PR already introduces) should help — the consumers (github_owner_repo, platform_for_repo) now delegate to it, so testing find_forge_remote directly covers the iteration logic. I'll work on closing the remaining coverage gaps.

[amodelaweb]

Thanks for the review @max-sixty !

Config alternative: Yeah, I think both approaches have merit. The config approach (forge = "github") is simpler and explicit — no magic. The insteadOf approach is zero-config but adds complexity. One middle ground: we could support both, with config taking precedence (like [ci] platform already does). But if you'd prefer config-only, I'm happy to simplify this PR to just add a forge config option and skip the insteadOf inference entirely. What's your preference?

Network requests: Zero — git remote get-url is purely local. It reads .git/config and applies url.insteadOf rewrites in-process. The only cost is one extra git subprocess spawn on the fallback path, and only when the raw hostname isn't recognized (so no impact for the common case).

Test coverage: Agreed. I've already consolidated the two-pass iteration into Repository::find_forge_remote() in the latest push, which makes the core logic directly testable. The remaining coverage gaps are in the thin consumer wrappers that delegate to it. I'll keep pushing on this.

Let me know which direction you'd like to go on the config vs insteadOf question — happy to adjust.

[max-sixty]

@worktrunk-bot please make those changes to improve the coverage

@amodelaweb thanks for the PR; please don't paste AI output from your handle. we have a bot for that!

[worktrunk-bot]

Added 6 tests covering the uncovered edge cases in the forge URL fallback code:

• github_push_url None paths — when push remote is GitLab (known forge, not GitHub) and when insteadOf resolves to GitLab
• find_forge_remote with no remotes — returns None when all remotes removed
• forge_remote_url insteadOf to unknown forge — returns raw URL when rewrite also maps to unrecognized host
• effective_remote_url / forge_remote_url for nonexistent remote — returns None

[max-sixty]

@worktrunk-bot please do a big consolidation, I see a bunch of repetition. run /simplify

[worktrunk-bot]

Consolidated the forge URL fallback logic (-72 lines net):

Source code:

• find_forge_remote: calls effective_remote_url directly instead of routing through forge_remote_url — avoids redundant re-parsing and re-checking of raw URLs already examined in the fast path
• github_push_url: delegates to forge_remote_url instead of manually reimplementing the known-forge check + fallback pattern

Tests:

• Extracted setup_insteadof / setup_push_tracking helpers to eliminate repeated 5-line setup blocks
• Merged the two nonexistent-remote tests into one
• Trimmed comments that just restated what the code does

All 492 unit + 48 default_branch integration tests pass. Waiting on CI.

[max-sixty]

@worktrunk-bot can we write a spec at the top of a module of exactly how we resolve what forge to use?

[worktrunk-bot]

Added a module-level spec to src/git/repository/remotes.rs covering:

• Why forge resolution exists (insteadOf aliases hide real hostnames)
• The algorithm — three steps: fast path (raw URL), insteadOf fallback (git remote get-url), best effort
• Entry points table mapping each function to its scope and consumers

Kept it concise — the spec describes the contract, individual doc comments on each function cover the details.

[max-sixty]

Why have logic in the caller? why not have all the logic in this one function, and otherwise fail?

[worktrunk-bot]

Good point — changed forge_remote_url to return None when neither the raw nor effective URL resolves to a known forge, instead of falling back to the raw URL. This lets all forge-resolution logic live inside that one function.

This also simplified github_push_url — it was manually doing a raw-URL check then falling back to forge_remote_url, but forge_remote_url already does both passes internally. Removed the duplicated logic and the now-unused push_remote_url method.

[max-sixty]

@worktrunk-bot note my question above

CI failed — snapshot mismatch + fmt lint

[worktrunk-bot]

CI is failing with two issues:

1. cargo fmt lint failure — a multi-line run_git call in test_forge_remote_url_known_forge_no_fallback needs to be collapsed to one line.

2. Snapshot mismatch in test_list_full_with_url_based_pushremote — this is a real regression from removing push_remote_url(). The test simulates gh pr checkout behavior where branch.feature.pushremote is set to a URL (https://github.com/fork-owner/test-repo.git), not a remote name.

The old push_remote_url() handled this via %(push:remotename) — if the result was already a URL (contains :// or starts with git@), it returned it directly. The new github_push_url() delegates to push_remote() which uses @{push} and extracts the remote name from the result. But @{push} fails when pushremote is a URL because there's no corresponding remote-tracking ref.

Fix options:

• Restore the URL-detection logic from push_remote_url() inside github_push_url() — check if push_remote() returns None, then fall back to reading %(push:remotename) and checking if it's a URL
• Or add a push_remote_url() method back that handles the URL-as-pushremote case, and call forge_remote_url only when it returns a remote name

[worktrunk-bot]

CI is failing — all three jobs (lint, test linux, test macos) fail on the same cargo fmt issue: the multi-line run_git call at tests/integration_tests/default_branch.rs:375-379 needs collapsing to a single line. This was flagged in a previous review but the latest push didn't include the fix.

The new push_remote_url() method and github_push_url() restructuring look correct — restoring URL-based pushremote support (for gh pr checkout fork PRs) while keeping insteadOf fallback via forge_remote_url().

Re @max-sixty's question about a config alternative (forge = "github"): the PR discussion surfaced this but it's still an open design question. The author offered to go either direction. If the preference is config-only, this PR's scope would shrink significantly. Worth deciding before further iteration on coverage/consolidation.

[worktrunk-bot]

Re the open design question — config vs insteadOf inference:

The two approaches aren't mutually exclusive, but I'd lean toward config-first (forge = "github" or extending the existing [ci] platform):

1. Explicit config is more predictable — insteadOf inference works when the rewrite points to a known forge, but silently gives up when it doesn't. A config value always works.
2. The existing [ci] platform config already solves half the problem — CI status detection already has a config path. Extending it (or adding forge) to cover wt switch pr: and push remote detection would be consistent.
3. insteadOf detection is a nice fallback — zero-config is genuinely better UX for the happy path. But it adds ~100 lines of code and subprocess spawns for what a one-line config would solve.

A reasonable path: add a forge config option as the primary mechanism, and keep the insteadOf inference as an optional fallback (or defer it to a follow-up if it's wanted later). That would shrink this PR significantly.

This is a design call — flagging for human decision.