refactor(tunnel): replace params with RunServicesOptions struct

[skevetter]

Signed-off-by: Samuel K skevetter@pm.me

Summary by CodeRabbit

• Refactor

  • Streamlined service startup and configuration handling for more predictable behavior.
  • Improved credential handling and Git SSH signing integration.

• Bug Fixes / Reliability

  • More robust retry/backoff and configurable exit timeout to reduce unexpected failures.
  • Improved port forwarding: clearer error reporting and more reliable forwarding of app and extra ports.

• Logging

  • Enhanced logs and user-facing error messages for troubleshooting.

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request refactors tunnel.RunServices to accept a single RunServicesOptions struct (replacing multiple positional parameters), updates all call sites to pass that struct, and substantially refactors pkg/tunnel/services.go to add retry/backoff, modularized port forwarding, tunnel server lifecycle management, and credentials command construction.

Changes

Cohort / File(s)	Summary
Call Site Updates cmd/ssh.go, cmd/up.go, pkg/client/clientimplementation/services.go	Call sites replaced multi-argument tunnel.RunServices(...) invocations with construction and passing of RunServicesOptions{...}.
API Definition & Implementation pkg/tunnel/services.go	Added exported RunServicesOptions struct and changed RunServices signature to RunServices(ctx, opts RunServicesOptions) error. Introduced retry/backoff constants and getExitAfterTimeout. Added tunnel server lifecycle helpers (tunnelServerParams, runTunnelServer, runServicesIteration), credentials command builders (buildCredentialsCommand, addGitSSHSigningKey), and modularized port forwarding (createForwarder, forwardDevContainerPorts, getContainerResult, forwardExtraPorts, forwardAppPorts, forwardConfigPorts, forwardPort, parseForwardPort). Logging and parsing improvements included.

Sequence Diagram(s)

sequenceDiagram
    participant CLI as rgba(30,144,255,0.5) CLI
    participant Local as rgba(34,139,34,0.5) LocalProcess
    participant Tunnel as rgba(255,165,0,0.5) TunnelServer
    participant Remote as rgba(128,0,128,0.5) RemoteCredentialsServer
    participant Container as rgba(220,20,60,0.5) Container

    CLI->>Local: calls RunServices(ctx, RunServicesOptions)
    Local->>Tunnel: startTunnelServer (runTunnelServer)
    Local->>Remote: buildCredentialsCommand -> exec remote credentials server
    Remote->>Container: request creds / configure git/docker
    Tunnel->>Container: establish port forwards (forwardDevContainerPorts -> forwardPort)
    Container-->>Remote: container result (ports, status)
    Remote-->>Local: exit / error (reported via channels)
    Local->>CLI: return error/exit based on retry/backoff and exit timeout

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• refactor(client): extract common client implementations to pkg #381: Mirrors the same RunServices API change (multi-argument → RunServicesOptions) and updates the same call sites.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the main objective of the PR: refactoring the tunnel RunServices function to accept parameters via a RunServicesOptions struct instead of multiple positional arguments.
Docstring Coverage	✅ Passed	Docstring coverage is 93.75% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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 refactor-tunnel-run-services-opts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@pkg/tunnel/services.go`:
- Around line 300-330: The function forwardConfigPorts can panic if
result.MergedConfig is nil; add a defensive nil-check at the start of
forwardConfigPorts (which takes portForwardParams p and *config2.Result result)
to return an empty []string immediately when result == nil or
result.MergedConfig == nil before accessing result.MergedConfig.ForwardPorts;
keep the existing behavior for parsing via parseForwardPort and async calls to
devssh.PortForward, and ensure any early-return path is consistent with
forwardAppPorts (no further goroutines or appends when nil).
- Around line 286-298: The functions forwardAppPorts and forwardConfigPorts
access result.MergedConfig without checking for nil; add a guard at the start of
each function that returns an empty []string if result == nil ||
result.MergedConfig == nil. Update forwardAppPorts (which iterates
result.MergedConfig.AppPort) and forwardConfigPorts (which reads
result.MergedConfig.ConfigPort) to short-circuit early to avoid nil-pointer
panics, keeping the rest of the logic unchanged.

🧹 Nitpick comments (1)

pkg/tunnel/services.go (1)

196-225: Consider adding more selective retry conditions.

The current retry logic retries all errors except context cancellation. This might cause unnecessary retries for permanent failures (e.g., authentication errors, permission denied). Consider filtering out non-transient errors:

}, func(err error) bool {
    if ctx.Err() != nil {
        return false
    }
    // Consider: check if error is transient (connection reset, timeout, etc.)
    // and return false for permanent errors
    return true
},

However, if the intent is maximum resilience for SSH connection instability, the current approach is acceptable.