Skip to content

Commit 57fc2cf

Browse files
authored
[Repo Assist] fix(mcp): replace hardcoded 30s with defaultConnectTimeout constant, fix <= 0 guard (#3946)
🤖 *This is a Repo Assist automated pull request.* ## Summary Closes #3933. Two locations in `internal/mcp/connection.go` used the magic literal `30 * time.Second` for the HTTP connect-timeout fallback. A canonical constant `config.DefaultConnectTimeout = 30` already exists in `internal/config/config_core.go`, but importing that package into `internal/mcp` would pull in `init()` functions with side effects. Instead, this PR introduces a local unexported constant to be the single source of truth within the package. It also fixes a latent bug: `reconnectSDKTransport` guarded with `== 0`, which left negative values through as-is. The guard is now `<= 0`, consistent with `NewHTTPConnection`. ## Root cause Issue #3933 (Duplicate Code Detector) identified: - `NewHTTPConnection` (line ~202): `if connectTimeout <= 0 { connectTimeout = 30 * time.Second }` - `reconnectSDKTransport` (line ~386): `if timeout == 0 { timeout = 30 * time.Second }` — note inconsistent guard Both hardcode 30 s instead of using a named constant, and the guards differ. ## Changes | File | Change | |------|--------| | `internal/mcp/connection.go` | Add `const defaultConnectTimeout = 30 * time.Second`; replace both magic literals with the constant; fix `== 0` → `<= 0` guard in `reconnectSDKTransport` | ## Trade-offs - **Why a local constant, not importing `config`?** `internal/config` has `init()` functions that register flags and load schemas. Importing it into `internal/mcp` just for one constant would be a heavyweight dependency and could cause ordering surprises. A comment on the constant explicitly documents the sync requirement. - **Behavioural change:** A negative `connectTimeout` passed to `reconnectSDKTransport` previously slipped through; it now falls back to 30 s. This is the correct behaviour (negative durations are invalid) and consistent with `NewHTTPConnection`. ## Test Status ⚠️ **Infrastructure constraint**: `proxy.golang.org` is blocked by the environment firewall, preventing `go test` from downloading the required Go toolchain (go1.25.0). The change is a pure constant-introduction refactor with no logic change for valid inputs; the `== 0` → `<= 0` guard fix is covered by the companion PR adding dedicated tests (`repo-assist/test-connect-timeout-defaults`). > [!WARNING] > <details> > <summary><strong>⚠️ Firewall blocked 1 domain</strong></summary> > > The following domain was blocked by the firewall during workflow execution: > > - `proxy.golang.org` > > To allow these domains, add them to the `network.allowed` list in your workflow frontmatter: > > ```yaml > network: > allowed: > - defaults > - "proxy.golang.org" > ``` > > See [Network Configuration](https://github.github.com/gh-aw/reference/network/) for more information. > > </details> > Generated by [Repo Assist](https://github.com/github/gh-aw-mcpg/actions/runs/24510723428/agentic_workflow) · ● 6.1M · [◷](https://github.com/search?q=repo%3Agithub%2Fgh-aw-mcpg+%22gh-aw-workflow-id%3A+repo-assist%22&type=pullrequests) > > To install this [agentic workflow](https://github.com/githubnext/agentics/blob/851905c06e905bf362a9f6cc54f912e3df747d55/workflows/repo-assist.md), run > ``` > gh aw add githubnext/agentics@851905c > ``` <!-- gh-aw-agentic-workflow: Repo Assist, engine: copilot, model: auto, id: 24510723428, workflow_id: repo-assist, run: https://github.com/github/gh-aw-mcpg/actions/runs/24510723428 --> <!-- gh-aw-workflow-id: repo-assist -->
2 parents daa3c76 + 1e5db88 commit 57fc2cf

1 file changed

Lines changed: 8 additions & 3 deletions

File tree

internal/mcp/connection.go

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,11 @@ import (
2424

2525
var logConn = logger.New("mcp:connection")
2626

27+
// defaultConnectTimeout is the fallback connect timeout used when the configured timeout
28+
// is non-positive or otherwise invalid.
29+
// Kept in sync with config.DefaultConnectTimeout (30 s) to avoid importing the config package.
30+
const defaultConnectTimeout = 30 * time.Second
31+
2732
// ContextKey for session ID
2833
type ContextKey string
2934

@@ -199,7 +204,7 @@ func NewConnection(ctx context.Context, serverID, command string, args []string,
199204
func NewHTTPConnection(ctx context.Context, serverID, url string, headers map[string]string, oidcProvider *oidc.Provider, oidcAudience string, keepAlive time.Duration, connectTimeout time.Duration) (*Connection, error) {
200205
// Apply default connect timeout when not specified
201206
if connectTimeout <= 0 {
202-
connectTimeout = 30 * time.Second
207+
connectTimeout = defaultConnectTimeout
203208
}
204209
logger.LogInfo("backend", "Creating HTTP MCP connection with transport fallback, url=%s, connectTimeout=%v", url, connectTimeout)
205210
ctx, cancel := context.WithCancel(ctx)
@@ -379,8 +384,8 @@ func (c *Connection) reconnectSDKTransport() error {
379384
}
380385

381386
timeout := c.connectTimeout
382-
if timeout == 0 {
383-
timeout = 30 * time.Second
387+
if timeout <= 0 {
388+
timeout = defaultConnectTimeout
384389
}
385390
connectCtx, cancel := context.WithTimeout(c.ctx, timeout)
386391
defer cancel()

0 commit comments

Comments
 (0)