Fix protocol port fallback for cases with port in input

[ehsandeep]

Fixes #2335

• Switch port 443→80 when falling back HTTPS→HTTP
• Switch port 80→443 when falling back HTTP→HTTPS
• Use net.JoinHostPort() for proper IPv6 support

Summary by CodeRabbit

• Bug Fixes

  • Failed HTTP requests now automatically retry with HTTPS, and vice versa, with automatic port adjustment for default ports (80↔443).

• Tests

  • Added unit tests for protocol fallback and port mapping logic.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Adds unit tests for port-to-scheme determination and fallback logic in the runner package. Implements automatic protocol and port switching in the HTTP/HTTPS retry flow: when fallback from HTTPS to HTTP is needed, port 443 switches to 80; HTTP to HTTPS fallback switches 80 to 443. Fixes incorrect HTTP requests to HTTPS ports causing 400 errors.

Changes

Cohort / File(s)	Summary
Test Coverage for Port Fallback runner/ports_optimization_test.go	Adds TestDetermineMostLikelySchemeOrder and TestSwitchPortForFallback unit tests. Validates scheme determination across default ports (80, 443, 8080, 8443) and tests getPortForFallback behavior for protocol switching with port adjustment logic.
Retry Flow Enhancement runner/runner.go	Modifies HTTP/HTTPS retry logic to switch between opposite protocols and adjust ports on fallback attempts. Implements conditional port mapping: HTTPS:443 → HTTP:80, HTTP:80 → HTTPS:443, other ports remain unchanged. Enables automatic protocol fallback before request abandonment.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

• Tests are straightforward and follow consistent patterns across cases
• Core retry logic change is localized to runner.go with clear conditional branches
• Port-switching logic is deterministic and easy to verify against the linked issue context
• No complex interactions or external dependencies introduced

Poem

🐰 A hop, skip, and port number dance—
When HTTPS fails, we give HTTP its chance!
Four-four-three to eighty, a graceful switch,
No more 400 errors in a protocol glitch! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check	✅ Passed	The PR implements the core requirement from issue #2335: switching ports during protocol fallback (443→80 for HTTPS→HTTP, 80→443 for HTTP→HTTPS), with tests validating this behavior and proper host construction.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to implementing protocol/port fallback logic: test file adds unit tests for the fallback mechanism, and runner.go implements the actual fallback behavior with port switching.
Title check	✅ Passed	The title 'Fix protocol port fallback for cases with port in input' directly aligns with the main change: fixing protocol/port fallback logic when input includes ports, preventing HTTP sent to HTTPS ports and enabling proper fallback behavior.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-protocol-port-fallback

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

runner/runner.go (1)

1756-1775: Protocol + port fallback logic matches requirements; consider aligning error-key port as a follow-up

The new fallback block correctly:

• Only triggers once for HTTPorHTTPS inputs (!retried && origProtocol == httpx.HTTPorHTTPS).
• Flips scheme and, when on default ports, switches 443 → 80 (HTTPS→HTTP) and 80 → 443 (HTTP→HTTPS).
• Uses net.JoinHostPort(URL.Hostname(), ...), which is the right choice for IPv6-safe target.Host reconstruction before re‑parsing.

One nuance to be aware of: hostPort (used with HostErrorsCache) is computed once before this block and not recomputed after you change URL and target.Host. When the fallback switches from :443 to :80 (or vice versa), any subsequent error is still counted under the original hostPort string. Functionally this is acceptable (the user-specified target host:port is what gets marked as “unresponsive”), but if you ever want HostMaxErrors to track the actual dialed port, you’ll need to recompute hostPort after adjusting URL and before incrementing the cache.

Given the current PR scope, the behavior is logically consistent and fixes the reported bug; the hostPort detail can be handled as a later cleanup if desired.

You may want to double-check how urlutil.URL populates Host vs Hostname() to ensure net.JoinHostPort(URL.Host, URL.Port()) (earlier in this function) never receives a host string that already contains a port. Verifying this against the current urlutil.ParseURL implementation and its docs would confirm there are no edge cases around IPv6 literals or host:port inputs.

runner/ports_optimization_test.go (1)

61-115: Port-fallback tests correctly encode 443↔80 behavior; watch for duplication with production logic

TestSwitchPortForFallback plus getPortForFallback accurately capture the intended mapping:

• HTTPS on 443 → HTTP on 80
• HTTP on 80 → HTTPS on 443
• Non-default ports (8080, 8443) remain unchanged

That matches the retry behavior in analyze and will prevent regressions in the default-port flip.

One thing to keep in mind is that getPortForFallback lives only in tests and duplicates the logic in runner.go. If the production fallback ever changes (e.g., additional ports or conditions), tests might still pass while validating only this helper. Longer term, you might consider moving this mapping into a small non-test helper and using it from both analyze and the tests, but for this PR the current approach is fine.

If you do extract a shared helper later, verify its behavior against the current test expectations (especially for non-default ports) and ensure you’re still using the httpx.HTTP/httpx.HTTPS constants rather than string literals, so changes in those constants don’t silently desynchronize the mapping.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f7712f7 and 11f2b0e.

📒 Files selected for processing (2)

• runner/ports_optimization_test.go (1 hunks)
• runner/runner.go (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

runner/runner.go (1)

common/httpx/http2.go (2)

• HTTPS (17-17)
• HTTP (15-15)

runner/ports_optimization_test.go (1)

common/httpx/http2.go (2)

• HTTP (15-15)
• HTTPS (17-17)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

• GitHub Check: Lint Test
• GitHub Check: release-test
• GitHub Check: Functional Test (ubuntu-latest)
• GitHub Check: Functional Test (macOS-latest)
• GitHub Check: Analyze (go)
• GitHub Check: Functional Test (windows-latest)

🔇 Additional comments (1)

runner/ports_optimization_test.go (1)

10-59: Scheme-selection tests look solid and capture key port heuristics

The table-driven coverage for determineMostLikelySchemeOrder is good: it locks in behavior for the main ports (80, 8080, 443, 8443), no-port hostnames, and the IP+port cases that triggered the original issue. This is a useful safety net around the scheme-guessing heuristics without overfitting to implementation details.

[dogancanbakir]

merge conflict

[ehsandeep]

Added in afd5951