From d04bdb8b1a3d1e8eaf4d2463e3c6f3ba1fc2f81c Mon Sep 17 00:00:00 2001 From: lux-liang <249971141+lux-liang@users.noreply.github.com> Date: Mon, 21 Sep 2026 01:18:43 +0800 Subject: [PATCH 1/3] Make timing-sensitive tests deterministic --- internal/security/mask_test.go | 32 ++++++--- internal/upstream/manager.go | 15 +++- .../upstream/manager_docker_recovery_test.go | 69 ++++++++----------- 3 files changed, 62 insertions(+), 54 deletions(-) diff --git a/internal/security/mask_test.go b/internal/security/mask_test.go index 40b22c88d..fa5ac30fe 100644 --- a/internal/security/mask_test.go +++ b/internal/security/mask_test.go @@ -4,7 +4,6 @@ import ( "fmt" "strings" "testing" - "time" "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" ) @@ -247,11 +246,9 @@ func TestMaskTextMasksBeyondTheDetectionCap(t *testing.T) { } } -// Removing the replacement cap makes "a payload stuffed with secrets" the -// worst case, so it must not be quadratic: a full-size activity response -// (64KB, the activity_max_response_size cap) of nothing but distinct tokens -// still has to mask in well under a second. -func TestMaskTextStaysCheapOnAPayloadFullOfSecrets(t *testing.T) { +// A full-size activity response made entirely of distinct tokens exercises the +// worst-case masking shape without making correctness depend on host timing. +func TestMaskTextHandlesAPayloadFullOfSecrets(t *testing.T) { d := NewDetector(nil) var b strings.Builder @@ -260,17 +257,30 @@ func TestMaskTextStaysCheapOnAPayloadFullOfSecrets(t *testing.T) { } text := b.String() - start := time.Now() masked, _ := d.MaskText(text) - elapsed := time.Since(start) if strings.Contains(masked, "ghp_0000") { t.Fatal("tokens survived masking") } - if elapsed > 2*time.Second { - t.Fatalf("masking a %d-byte payload took %s", len(text), elapsed) +} + +func BenchmarkMaskTextPayloadFullOfSecrets(b *testing.B) { + d := NewDetector(nil) + + var payload strings.Builder + for i := 0; payload.Len() < 64*1024; i++ { + fmt.Fprintf(&payload, "key%d=ghp_%036d\n", i, i) + } + text := payload.String() + + b.SetBytes(int64(len(text))) + b.ReportAllocs() + b.ResetTimer() + for range b.N { + if _, changed := d.MaskText(text); !changed { + b.Fatal("payload was not masked") + } } - t.Logf("masked %d bytes in %s", len(text), elapsed) } // Turning detection off later must not retroactively serve the credentials in diff --git a/internal/upstream/manager.go b/internal/upstream/manager.go index 7cc5f1147..ad5787374 100644 --- a/internal/upstream/manager.go +++ b/internal/upstream/manager.go @@ -46,6 +46,10 @@ var dockerResolverFn = func(logger *zap.Logger) (string, error) { return shellwrap.ResolveDockerPath(logger) } +func runDockerInfo(ctx context.Context, dockerBin string) error { + return exec.CommandContext(ctx, dockerBin, "info", "--format", "{{json .ServerVersion}}").Run() +} + // Docker recovery constants - internal implementation defaults const ( dockerCheckInterval = 30 * time.Second // How often to check Docker availability @@ -135,6 +139,7 @@ type Manager struct { dockerRecoveryState *storage.DockerRecoveryState dockerRecoveryCancel context.CancelFunc storageMgr *storage.Manager // Reference to storage manager for Docker state persistence + dockerInfoRunner func(context.Context, string) error // Tool discovery callback for notifications/tools/list_changed handling toolDiscoveryCallback func(ctx context.Context, serverName string) error @@ -215,6 +220,7 @@ func NewManager(logger *zap.Logger, globalConfig *config.Config, boltStorage *st shutdownCtx: shutdownCtx, shutdownCancel: shutdownCancel, storageMgr: storageMgr, + dockerInfoRunner: runDockerInfo, limiters: limiter.NewRegistry(), } manager.globalConfig.Store(globalConfig) @@ -3035,8 +3041,13 @@ func (m *Manager) checkDockerAvailability(ctx context.Context) error { dockerBin = "docker" } - cmd := exec.CommandContext(checkCtx, dockerBin, "info", "--format", "{{json .ServerVersion}}") - if err := cmd.Run(); err != nil { + runInfo := m.dockerInfoRunner + if runInfo == nil { + // Keep zero-value Managers usable in focused tests and small callers. + runInfo = runDockerInfo + } + + if err := runInfo(checkCtx, dockerBin); err != nil { return fmt.Errorf("docker unavailable: %w", err) } return nil diff --git a/internal/upstream/manager_docker_recovery_test.go b/internal/upstream/manager_docker_recovery_test.go index a3fc35473..d1a340ae6 100644 --- a/internal/upstream/manager_docker_recovery_test.go +++ b/internal/upstream/manager_docker_recovery_test.go @@ -3,7 +3,6 @@ package upstream import ( "context" "errors" - "os" "path/filepath" "runtime" "testing" @@ -21,30 +20,14 @@ import ( // now goes through dockerResolverFn (shellwrap-backed) instead of relying on // $PATH for the bare "docker" binary. // -// We exercise the real resolver indirectly: the test installs a fake docker -// shim into a temp dir, points dockerResolverFn at it, drops $PATH to the -// launchd minimum, and asserts that checkDockerAvailability still succeeds. -// Without the fix, exec.Command("docker") would error with `executable file -// not found in $PATH` regardless of how the resolver was wired. +// The command runner is injected so the test asserts the resolved executable +// directly without depending on process startup or a wall-clock deadline. func TestCheckDockerAvailability_UsesShellwrapResolver(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("launchd PATH scenario is macOS/Linux only") } - tmpDir := t.TempDir() - dockerPath := filepath.Join(tmpDir, "docker") - - // Fake docker that prints a server version when invoked with `info`. - // Anything else exits non-zero so we know the call shape was correct. - script := `#!/bin/sh -case "$1" in - info) printf '"24.0.0"\n'; exit 0 ;; - *) exit 99 ;; -esac -` - if err := os.WriteFile(dockerPath, []byte(script), 0o755); err != nil { - t.Fatalf("write fake docker: %v", err) - } + dockerPath := filepath.Join(t.TempDir(), "docker") // Simulate launchd's minimal PATH — fake docker is NOT on it. If the // production code resolved via os.Getenv("PATH") we would fail here. @@ -56,17 +39,25 @@ esac return dockerPath, nil } - m := &Manager{logger: zap.NewNop()} - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) - defer cancel() + var resolvedBin string + m := &Manager{ + logger: zap.NewNop(), + dockerInfoRunner: func(_ context.Context, dockerBin string) error { + resolvedBin = dockerBin + return nil + }, + } - if err := m.checkDockerAvailability(ctx); err != nil { + if err := m.checkDockerAvailability(context.Background()); err != nil { t.Fatalf("expected docker to be reachable via resolver, got: %v", err) } + if resolvedBin != dockerPath { + t.Fatalf("docker executable = %q, want resolved path %q", resolvedBin, dockerPath) + } } // TestCheckDockerAvailability_FallbackOnResolverFailure ensures that even when -// the resolver itself errors out we still attempt a bare-name exec — preserving +// the resolver itself errors out we still select the bare executable name — preserving // the original behaviour for hosts where the shellwrap probes legitimately // cannot find docker but it IS on the parent's PATH. func TestCheckDockerAvailability_FallbackOnResolverFailure(t *testing.T) { @@ -74,31 +65,27 @@ func TestCheckDockerAvailability_FallbackOnResolverFailure(t *testing.T) { t.Skip("/bin/sh-style fake docker is POSIX only") } - tmpDir := t.TempDir() - dockerPath := filepath.Join(tmpDir, "docker") - script := `#!/bin/sh -exit 0 -` - if err := os.WriteFile(dockerPath, []byte(script), 0o755); err != nil { - t.Fatalf("write fake docker: %v", err) - } - - // Resolver fails — the fallback should still exec via bare PATH. - t.Setenv("PATH", tmpDir) - original := dockerResolverFn t.Cleanup(func() { dockerResolverFn = original }) dockerResolverFn = func(*zap.Logger) (string, error) { return "", errors.New("simulated resolver failure") } - m := &Manager{logger: zap.NewNop()} - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) - defer cancel() + var resolvedBin string + m := &Manager{ + logger: zap.NewNop(), + dockerInfoRunner: func(_ context.Context, dockerBin string) error { + resolvedBin = dockerBin + return nil + }, + } - if err := m.checkDockerAvailability(ctx); err != nil { + if err := m.checkDockerAvailability(context.Background()); err != nil { t.Fatalf("expected fallback bare-name exec to succeed, got: %v", err) } + if resolvedBin != "docker" { + t.Fatalf("docker executable = %q, want bare-name fallback", resolvedBin) + } } // TestFreshenLoadedDockerRecoveryState verifies that a state loaded from the From e23be1a83da49d13bf5cc21f0002cb1394e46d06 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Mon, 21 Sep 2026 14:02:41 +0300 Subject: [PATCH 2/3] fix: restore real-exec/perf-regression coverage lost by hermetic test rewrite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code review of the deflaking changes found the hermeticity fixes dropped guarantees rather than relocating them: - runDockerInfo's real exec.CommandContext(...).Run() invocation shape had zero test coverage (both docker-recovery tests injected a fake runner). Add TestRunDockerInfo_ExecutesResolvedBinary and TestRunDockerInfo_PropagatesNonZeroExit, which exercise the real subprocess path directly and hermetically (no PATH tricks, no wall clock beyond a generous timeout). - The quadratic-time regression guard for MaskText was replaced by a benchmark nothing in CI ever runs. Add TestMaskTextScalesLinearlyOnAPayloadFullOfSecrets, which compares relative growth across payload sizes instead of an absolute wall-clock ceiling, so it stays sensitive to a real O(n^2) regression without the flakiness that motivated removing the original timing assertion. - Unify the two DI idioms checkDockerAvailability used for sibling fakes (dockerResolverFn package var vs dockerInfoRunner struct field with a nil-fallback) into one: dockerInfoRunnerFn, a package var mirroring dockerResolverFn. Removes the dead nil-fallback branch entirely. - Drop the leftover t.Setenv("PATH", ...) and stale comment in TestCheckDockerAvailability_UsesShellwrapResolver — the injected runner never execs, so PATH was no longer load-bearing for that test. - Extract secretsPayload() so the correctness test, the new complexity guard, and the benchmark build the same payload shape instead of three copies that could silently drift apart. Co-Authored-By: Claude Sonnet 5 --- internal/security/mask_test.go | 60 ++++++++++--- internal/upstream/manager.go | 16 ++-- .../upstream/manager_docker_recovery_test.go | 86 +++++++++++++++---- 3 files changed, 124 insertions(+), 38 deletions(-) diff --git a/internal/security/mask_test.go b/internal/security/mask_test.go index fa5ac30fe..7c1683ae3 100644 --- a/internal/security/mask_test.go +++ b/internal/security/mask_test.go @@ -4,6 +4,7 @@ import ( "fmt" "strings" "testing" + "time" "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" ) @@ -246,16 +247,24 @@ func TestMaskTextMasksBeyondTheDetectionCap(t *testing.T) { } } +// secretsPayload builds text of at least targetLen bytes made entirely of +// distinct GitHub-PAT-shaped tokens ("key=ghp_"), +// the worst case for masking: every token is a separate match, none share a +// value, so no dedup/caching in the detector can shortcut the work. +func secretsPayload(targetLen int) string { + var b strings.Builder + for i := 0; b.Len() < targetLen; i++ { + fmt.Fprintf(&b, "key%d=ghp_%036d\n", i, i) + } + return b.String() +} + // A full-size activity response made entirely of distinct tokens exercises the // worst-case masking shape without making correctness depend on host timing. func TestMaskTextHandlesAPayloadFullOfSecrets(t *testing.T) { d := NewDetector(nil) - var b strings.Builder - for i := 0; b.Len() < 64*1024; i++ { - fmt.Fprintf(&b, "key%d=ghp_%036d\n", i, i) - } - text := b.String() + text := secretsPayload(64 * 1024) masked, _ := d.MaskText(text) @@ -264,14 +273,43 @@ func TestMaskTextHandlesAPayloadFullOfSecrets(t *testing.T) { } } -func BenchmarkMaskTextPayloadFullOfSecrets(b *testing.B) { - d := NewDetector(nil) +// TestMaskTextScalesLinearlyOnAPayloadFullOfSecrets guards against +// reintroducing the O(n^2) behavior TestMaskTextStaysCheapOnAPayloadFullOfSecrets +// used to catch (removed because an absolute wall-clock ceiling was flaky under +// CI load — see BenchmarkMaskTextPayloadFullOfSecrets below for the profiling +// counterpart). Instead of a fixed threshold, it compares masking a payload +// against one 4x the size: linear work predicts ~4x the time, so a generous +// growth ceiling still catches real quadratic blowups (~16x) while absorbing +// routine scheduler/GC noise from the comparison running in the same process. +func TestMaskTextScalesLinearlyOnAPayloadFullOfSecrets(t *testing.T) { + if testing.Short() { + t.Skip("timing-sensitive; skipped under -short") + } + + measure := func(targetLen int) time.Duration { + d := NewDetector(nil) + text := secretsPayload(targetLen) + start := time.Now() + d.MaskText(text) + return time.Since(start) + } + + // Warm up (first run pays for allocator/CPU-cache warmup, not algorithmic cost). + measure(16 * 1024) - var payload strings.Builder - for i := 0; payload.Len() < 64*1024; i++ { - fmt.Fprintf(&payload, "key%d=ghp_%036d\n", i, i) + small := measure(16 * 1024) + large := measure(64 * 1024) // 4x the payload + + // The absolute floor keeps this from firing on noise when both runs are + // too fast for the ratio to mean anything. + if large > 10*small && large > 50*time.Millisecond { + t.Fatalf("masking a payload 4x the size took %s vs %s for the baseline — looks like a complexity regression, not scheduler noise", large, small) } - text := payload.String() +} + +func BenchmarkMaskTextPayloadFullOfSecrets(b *testing.B) { + d := NewDetector(nil) + text := secretsPayload(64 * 1024) b.SetBytes(int64(len(text))) b.ReportAllocs() diff --git a/internal/upstream/manager.go b/internal/upstream/manager.go index ad5787374..ba1341da1 100644 --- a/internal/upstream/manager.go +++ b/internal/upstream/manager.go @@ -50,6 +50,12 @@ func runDockerInfo(ctx context.Context, dockerBin string) error { return exec.CommandContext(ctx, dockerBin, "info", "--format", "{{json .ServerVersion}}").Run() } +// dockerInfoRunnerFn runs the resolved docker binary's `info` command. +// Overridable in tests, mirroring dockerResolverFn above so the two sibling +// fakes checkDockerAvailability needs (which binary, and running it) share one +// injection idiom instead of two. +var dockerInfoRunnerFn = runDockerInfo + // Docker recovery constants - internal implementation defaults const ( dockerCheckInterval = 30 * time.Second // How often to check Docker availability @@ -139,7 +145,6 @@ type Manager struct { dockerRecoveryState *storage.DockerRecoveryState dockerRecoveryCancel context.CancelFunc storageMgr *storage.Manager // Reference to storage manager for Docker state persistence - dockerInfoRunner func(context.Context, string) error // Tool discovery callback for notifications/tools/list_changed handling toolDiscoveryCallback func(ctx context.Context, serverName string) error @@ -220,7 +225,6 @@ func NewManager(logger *zap.Logger, globalConfig *config.Config, boltStorage *st shutdownCtx: shutdownCtx, shutdownCancel: shutdownCancel, storageMgr: storageMgr, - dockerInfoRunner: runDockerInfo, limiters: limiter.NewRegistry(), } manager.globalConfig.Store(globalConfig) @@ -3041,13 +3045,7 @@ func (m *Manager) checkDockerAvailability(ctx context.Context) error { dockerBin = "docker" } - runInfo := m.dockerInfoRunner - if runInfo == nil { - // Keep zero-value Managers usable in focused tests and small callers. - runInfo = runDockerInfo - } - - if err := runInfo(checkCtx, dockerBin); err != nil { + if err := dockerInfoRunnerFn(checkCtx, dockerBin); err != nil { return fmt.Errorf("docker unavailable: %w", err) } return nil diff --git a/internal/upstream/manager_docker_recovery_test.go b/internal/upstream/manager_docker_recovery_test.go index d1a340ae6..acbae5dbb 100644 --- a/internal/upstream/manager_docker_recovery_test.go +++ b/internal/upstream/manager_docker_recovery_test.go @@ -3,6 +3,7 @@ package upstream import ( "context" "errors" + "os" "path/filepath" "runtime" "testing" @@ -14,14 +15,65 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" ) +// TestRunDockerInfo_ExecutesResolvedBinary exercises runDockerInfo's real +// exec.CommandContext(...).Run() invocation shape end-to-end against a real +// subprocess, so a regression in the argument/flag shape (wrong dockerBin +// position, broken --format string) is caught even though the tests below +// inject dockerInfoRunnerFn and never call this function. +func TestRunDockerInfo_ExecutesResolvedBinary(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("/bin/sh-style fake docker is POSIX only") + } + + dockerPath := filepath.Join(t.TempDir(), "docker") + script := `#!/bin/sh +case "$1" in + info) printf '"24.0.0"\n'; exit 0 ;; + *) exit 99 ;; +esac +` + if err := os.WriteFile(dockerPath, []byte(script), 0o755); err != nil { + t.Fatalf("write fake docker: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + if err := runDockerInfo(ctx, dockerPath); err != nil { + t.Fatalf("expected fake docker info to succeed, got: %v", err) + } +} + +// TestRunDockerInfo_PropagatesNonZeroExit ensures a real non-zero process exit +// (not just a synthetic Go error) surfaces as an error from runDockerInfo. +func TestRunDockerInfo_PropagatesNonZeroExit(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("/bin/sh-style fake docker is POSIX only") + } + + dockerPath := filepath.Join(t.TempDir(), "docker") + if err := os.WriteFile(dockerPath, []byte("#!/bin/sh\nexit 7\n"), 0o755); err != nil { + t.Fatalf("write fake docker: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + if err := runDockerInfo(ctx, dockerPath); err == nil { + t.Fatal("expected non-zero exit to surface as an error") + } +} + // TestCheckDockerAvailability_UsesShellwrapResolver verifies that a launchd-style // minimal PATH (the situation when mcpproxy is launched from /Applications/...app // or a LoginItem) does not break docker lookup, because checkDockerAvailability // now goes through dockerResolverFn (shellwrap-backed) instead of relying on // $PATH for the bare "docker" binary. // -// The command runner is injected so the test asserts the resolved executable -// directly without depending on process startup or a wall-clock deadline. +// dockerInfoRunnerFn is faked so the test asserts the resolved executable +// directly without depending on process startup or a wall-clock deadline; the +// real exec.CommandContext(...).Run() shape is covered separately by +// TestRunDockerInfo_ExecutesResolvedBinary above. func TestCheckDockerAvailability_UsesShellwrapResolver(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("launchd PATH scenario is macOS/Linux only") @@ -29,10 +81,6 @@ func TestCheckDockerAvailability_UsesShellwrapResolver(t *testing.T) { dockerPath := filepath.Join(t.TempDir(), "docker") - // Simulate launchd's minimal PATH — fake docker is NOT on it. If the - // production code resolved via os.Getenv("PATH") we would fail here. - t.Setenv("PATH", "/usr/bin:/bin:/usr/sbin:/sbin") - original := dockerResolverFn t.Cleanup(func() { dockerResolverFn = original }) dockerResolverFn = func(*zap.Logger) (string, error) { @@ -40,14 +88,15 @@ func TestCheckDockerAvailability_UsesShellwrapResolver(t *testing.T) { } var resolvedBin string - m := &Manager{ - logger: zap.NewNop(), - dockerInfoRunner: func(_ context.Context, dockerBin string) error { - resolvedBin = dockerBin - return nil - }, + originalRunner := dockerInfoRunnerFn + t.Cleanup(func() { dockerInfoRunnerFn = originalRunner }) + dockerInfoRunnerFn = func(_ context.Context, dockerBin string) error { + resolvedBin = dockerBin + return nil } + m := &Manager{logger: zap.NewNop()} + if err := m.checkDockerAvailability(context.Background()); err != nil { t.Fatalf("expected docker to be reachable via resolver, got: %v", err) } @@ -72,14 +121,15 @@ func TestCheckDockerAvailability_FallbackOnResolverFailure(t *testing.T) { } var resolvedBin string - m := &Manager{ - logger: zap.NewNop(), - dockerInfoRunner: func(_ context.Context, dockerBin string) error { - resolvedBin = dockerBin - return nil - }, + originalRunner := dockerInfoRunnerFn + t.Cleanup(func() { dockerInfoRunnerFn = originalRunner }) + dockerInfoRunnerFn = func(_ context.Context, dockerBin string) error { + resolvedBin = dockerBin + return nil } + m := &Manager{logger: zap.NewNop()} + if err := m.checkDockerAvailability(context.Background()); err != nil { t.Fatalf("expected fallback bare-name exec to succeed, got: %v", err) } From de2b4b47193449912204061d84d2913b7b60680a Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Mon, 21 Sep 2026 14:11:01 +0300 Subject: [PATCH 3/3] fix: make the fake docker in TestRunDockerInfo_ExecutesResolvedBinary validate full argv Cross-review (codex/gpt-5.6-sol) round 1 finding: the fake docker script only checked $1 == "info", so it would still exit 0 if the --format flag or the Go template string were deleted or corrupted, silently defeating the test's stated purpose of catching that exact regression. Require all three arguments match exactly. Co-Authored-By: Claude Sonnet 5 --- internal/upstream/manager_docker_recovery_test.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/internal/upstream/manager_docker_recovery_test.go b/internal/upstream/manager_docker_recovery_test.go index acbae5dbb..f744922b5 100644 --- a/internal/upstream/manager_docker_recovery_test.go +++ b/internal/upstream/manager_docker_recovery_test.go @@ -26,11 +26,15 @@ func TestRunDockerInfo_ExecutesResolvedBinary(t *testing.T) { } dockerPath := filepath.Join(t.TempDir(), "docker") + // Requires the exact argv runDockerInfo passes (not just $1 == info), so a + // regression in argument count, the --format flag, or the Go template + // string fails this test instead of slipping through unnoticed. script := `#!/bin/sh -case "$1" in - info) printf '"24.0.0"\n'; exit 0 ;; - *) exit 99 ;; -esac +if [ "$#" -eq 3 ] && [ "$1" = "info" ] && [ "$2" = "--format" ] && [ "$3" = '{{json .ServerVersion}}' ]; then + printf '"24.0.0"\n' + exit 0 +fi +exit 99 ` if err := os.WriteFile(dockerPath, []byte(script), 0o755); err != nil { t.Fatalf("write fake docker: %v", err)