Skip to content

Commit 9fc5630

Browse files
Dumbrisclaude
andcommitted
fix(update): tray gate honors --config override; check() re-reads enabled
Codex final-review findings on PR #805 (Spec 079 US1): - Major: ServerAdapter.GetConfigPath() hardcoded ~/.mcpproxy/mcp_config.json, ignoring MCPPROXY_TRAY_CONFIG_PATH — the very path the tray launches core with as --config (buildCoreArgs). So the update_check gate read the wrong file under a custom config path and failed open, letting the tray-owned daily GitHub check run despite update_check.enabled=false. Resolve the env override first, matching what core actually uses. - Minor: check() captured cfgGen but not the enabled flag, so a disable racing after the caller's outer Enabled() gate (loop tick / re-enable goroutine) still issued a GitHub request. The generation guard dropped the result, but the request fired — violating FR-015 "no network check on any surface". Re-read enabled under the same lock as gen and bail before checkFunc. Both covered by new tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 46087d6 commit 9fc5630

4 files changed

Lines changed: 70 additions & 1 deletion

File tree

cmd/mcpproxy-tray/internal/api/adapter.go

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"os"
77
"path/filepath"
88
"runtime"
9+
"strings"
910

1011
internalRuntime "github.com/smart-mcp-proxy/mcpproxy-go/internal/runtime"
1112
"github.com/smart-mcp-proxy/mcpproxy-go/internal/tray"
@@ -307,8 +308,15 @@ func (a *ServerAdapter) ReloadConfiguration() error {
307308
return fmt.Errorf("ReloadConfiguration not yet supported via API")
308309
}
309310

310-
// GetConfigPath returns the configuration file path
311+
// GetConfigPath returns the configuration file path core is running with.
312+
// The tray passes MCPPROXY_TRAY_CONFIG_PATH to core as --config (see
313+
// buildCoreArgs in main.go), so tray-side config consumers — e.g. the Spec 079
314+
// update_check gate — must resolve to that same override, not a hardcoded
315+
// default. Falls back to the default ~/.mcpproxy path when unset.
311316
func (a *ServerAdapter) GetConfigPath() string {
317+
if cfg := strings.TrimSpace(os.Getenv("MCPPROXY_TRAY_CONFIG_PATH")); cfg != "" {
318+
return cfg
319+
}
312320
homeDir, err := os.UserHomeDir()
313321
if err != nil {
314322
return "~/.mcpproxy/mcp_config.json" // fallback

cmd/mcpproxy-tray/internal/api/adapter_test.go

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -645,3 +645,30 @@ func TestHealthDataFlow_EndToEnd(t *testing.T) {
645645
status := adapter.GetStatus().(map[string]interface{})
646646
assert.Equal(t, 1, status["connected_servers"], "Status should use health.level for connected count")
647647
}
648+
649+
// =============================================================================
650+
// ServerAdapter.GetConfigPath Tests
651+
// =============================================================================
652+
653+
// The tray launches core with --config <MCPPROXY_TRAY_CONFIG_PATH> (see
654+
// buildCoreArgs in main.go). GetConfigPath must resolve to that SAME path so
655+
// tray-side config consumers (e.g. the Spec 079 update_check gate) read the
656+
// config core is actually using — not a hardcoded default.
657+
func TestServerAdapter_GetConfigPath_HonorsTrayConfigPathEnv(t *testing.T) {
658+
const custom = "/tmp/custom-tray-config/mcp_config.json"
659+
t.Setenv("MCPPROXY_TRAY_CONFIG_PATH", custom)
660+
661+
adapter := NewServerAdapter(NewMockClient())
662+
663+
assert.Equal(t, custom, adapter.GetConfigPath())
664+
}
665+
666+
func TestServerAdapter_GetConfigPath_DefaultWhenEnvUnset(t *testing.T) {
667+
t.Setenv("MCPPROXY_TRAY_CONFIG_PATH", "")
668+
669+
adapter := NewServerAdapter(NewMockClient())
670+
671+
got := adapter.GetConfigPath()
672+
assert.Contains(t, got, "mcp_config.json")
673+
assert.NotEqual(t, "", got)
674+
}

internal/updatecheck/checker.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -244,8 +244,19 @@ func (c *Checker) check() {
244244

245245
c.mu.RLock()
246246
gen := c.cfgGen
247+
enabled := c.enabledLocked()
247248
c.mu.RUnlock()
248249

250+
// Re-read enabled under the same lock as gen: a disable racing after the
251+
// caller's outer Enabled() gate (loop tick, or a re-enable-triggered
252+
// goroutine from SetConfig) must not fire a network request. The
253+
// generation guard already drops any stale result; this avoids the request
254+
// entirely (FR-015: disabled means no network check on any surface).
255+
if !enabled {
256+
c.logger.Debug("Skipping update check: update checking disabled")
257+
return
258+
}
259+
249260
release, err := c.checkFunc()
250261
if err != nil {
251262
c.logger.Debug("Update check failed", zap.Error(err))

internal/updatecheck/checker_test.go

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,29 @@ func TestChecker_SetConfig_DisabledSkipsCheckAndHidesInfo(t *testing.T) {
211211
}
212212
}
213213

214+
// TestChecker_CheckSkipsNetworkWhenDisabled verifies the low-level check()
215+
// loop path re-reads the enabled flag before hitting the network, so a disable
216+
// racing an in-flight tick or a re-enable-triggered goroutine performs no
217+
// GitHub request (FR-015: disabled means no network check on any surface).
218+
func TestChecker_CheckSkipsNetworkWhenDisabled(t *testing.T) {
219+
checker := New(zaptest.NewLogger(t), "v1.0.0")
220+
221+
calls := 0
222+
checker.SetCheckFunc(func() (*GitHubRelease, error) {
223+
calls++
224+
return &GitHubRelease{TagName: "v1.1.0"}, nil
225+
})
226+
227+
checker.SetConfig(false, false)
228+
229+
// Directly exercise the loop's check() (bypasses the CheckNow gate).
230+
checker.check()
231+
232+
if calls != 0 {
233+
t.Errorf("check function invoked %d times, want 0 when disabled", calls)
234+
}
235+
}
236+
214237
// TestChecker_SetConfig_ReEnableRestoresChecks verifies a hot-reload flip back
215238
// to enabled=true makes CheckNow work again without a restart (FR-012).
216239
func TestChecker_SetConfig_ReEnableRestoresChecks(t *testing.T) {

0 commit comments

Comments
 (0)