[test] Add tests for launcher.GetOrLaunchForSession and config.EffectiveContainerRuntimeCommand - #10406
Conversation
…ainerRuntimeCommand - Add TestGetOrLaunchForSession_DoubleCheckLockHit: tests the double-check locking pattern where a concurrent goroutine pre-populates the session pool while another goroutine is blocked waiting for the mutex - Add TestGetOrLaunchForSession_PoolStop_CleansDrained: tests that Launcher.Close() drains the session connection pool - Add TestGetOrLaunchForSession_StartupTimeout: tests the launchStdioConnection timeout path when a stdio command hangs during MCP handshake - Add TestEffectiveContainerRuntimeCommand_PublicWrapper: covers the exported EffectiveContainerRuntimeCommand function (was 0% coverage), testing nil gateway, docker, podman, and explicit command path override cases Coverage improvements: - config.EffectiveContainerRuntimeCommand: 0% → 100% - launcher.GetOrLaunchForSession: adds coverage for double-check lock and startup timeout paths - launcher.launchStdioConnection: adds coverage for timeout error path Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds targeted tests for session-aware launcher behavior and the exported container-runtime command wrapper.
Changes:
- Tests concurrent session-pool lookup, pool cleanup, and startup timeout handling.
- Tests
EffectiveContainerRuntimeCommandacross common runtime configurations.
Show a summary per file
| File | Description |
|---|---|
internal/launcher/getorlaunchforsession_test.go |
Adds launcher concurrency, cleanup, and timeout tests. |
internal/config/effective_container_runtime_command_test.go |
Adds coverage for the exported runtime-command wrapper. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 4
- Review effort level: Balanced
| // Give the goroutine time to reach the l.mu.Lock() call. | ||
| time.Sleep(50 * time.Millisecond) |
| // TestEffectiveContainerRuntimeCommand_PublicWrapper verifies that the exported | ||
| // EffectiveContainerRuntimeCommand function delegates to the private implementation | ||
| // and returns the same result. | ||
| func TestEffectiveContainerRuntimeCommand_PublicWrapper(t *testing.T) { |
| // TestGetOrLaunchForSession_StopTimesOut tests the Stop method timeout path when the | ||
| // cleanup goroutine does not finish within one second. |
| func TestGetOrLaunchForSession_StartupTimeout(t *testing.T) { | ||
| require := require.New(t) | ||
|
|
||
| ctx := context.Background() |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@copilot address review feedback |
…, cancellable context, hermetic env test
All four review items addressed in commit
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Investigated the CI failures in run 30593323479:
|
🔒 mcpg Read-Only Stress — gVisorSurface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Overall: PASS
References: §30606135675
|
🔒 mcpg Read-Only Stress — default AWFSurface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Overall: PASS Full ReportPart A — MCP reads (ALLOWED ✅)
Part B — MCP writes (BLOCKED ✅) Gateway exposes exactly 22 read-only tools (confirmed via
Note: refusal is via gateway tool registry exclusion (MCP -32602). The gateway connects to the backend (status=200) and rejects at its own tool dispatch layer. Part C — Proxied CLI reads (ALLOWED ✅)
Part D — CLI REST writes (BLOCKED ✅)
Part E — GraphQL mutations (BLOCKED ✅)
Run: §30606135638
|
🔒 mcpg Read-Only Stress — docker-sbxSurface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Overall: PASS Notes:
|
|
@copilot fix failing ci tests |
The public EffectiveContainerRuntimeCommand function in internal/config/container_runtime.go had 0% test coverage because all existing tests exercised only the private effectiveContainerRuntimeCommand helper directly. Add TestEffectiveContainerRuntimeCommand_PublicWrapper with four subtests covering the key behaviours delegated by the wrapper: - nil gateway → default docker command - explicit podman ContainerRuntime → podman command - MCP_GATEWAY_CONTAINER_RUNTIME env override - ContainerRuntimeCommand field wins over env override Coverage for container_runtime.go:EffectiveContainerRuntimeCommand rises from 0% to 100%. Overall internal/config package coverage improves from 97.8% to 97.9%. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…in tests, document best practice in AGENTS.md
Add missing success-path debug logs to several resolver functions: - resolveServiceName: log when using default service name - resolveExporterTimeout: log when using default timeout - resolveJSONExtraEndpoints: log parsed endpoint count and per-endpoint header count Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…credential leakage
Add two test functions that cover the previously untested PR review write arm in apply_tool_labels: - apply_tool_labels_pr_review_write_tools_are_repo_scoped_writes: covers the 7 granular MCP tool names (add_pull_request_review_comment, etc.) - apply_tool_labels_pr_review_legacy_aliases_are_repo_scoped_writes: covers the 3 legacy/synthetic aliases (pull_request_review_write, etc.) Both tests mirror the existing reaction_operations and workflow_toggle patterns. Closes #10393. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… untested legacy aliases
- Fix Quick Start note: gateway section and all fields are optional (not required) - Add reference to optional server fields with link to docs/CONFIGURATION.md - Add dockerless, containerRuntime, containerRuntimeCommand, containerRuntimeArgs to Gateway Configuration table - Note dockerless is JSON stdin only - Fix CONTRIBUTING.md: make test-integration auto-builds binary; make test-all always rebuilds - Add OTEL_EXPORTER_OTLP_ENDPOINT and GITHUB_REPOSITORY to env vars section - Update footer note to mention shared fields (registry) alongside TOML-only fields Closes #10373
… to run.sh in CONTRIBUTING.md
Add TestCheckContainerRuntimeAccessible_DockerBranch which exercises the code path in CheckContainerRuntimeAccessible where the command basename is "docker", causing it to delegate to CheckDockerAccessible instead of running the command directly. Before: CheckContainerRuntimeAccessible = 88.9% coverage After: CheckContainerRuntimeAccessible = 100.0% coverage Package total: 99.1% → 100.0% Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Reorder TestEffectiveContainerRuntimeCommand_PublicWrapper to match main's position (before TestRuntimeCommandForName), eliminating the duplicate that appeared in the PR merged test run - Remove duplicate apply_tool_labels_pr_review_legacy_aliases_are_repo_scoped_writes from tool_rules.rs; the second copy (testing only 2 ops) was redundant with the first copy (testing 3 ops including pull_request_review_write)
Fixed in commit Two root causes found:
|
Test Coverage Improvement
Functions Targeted
internal/launcher,internal/configGetOrLaunchForSession,launchStdioConnection,EffectiveContainerRuntimeCommandEffectiveContainerRuntimeCommand0%,GetOrLaunchForSession80.8%EffectiveContainerRuntimeCommand100%Why These Functions?
GetOrLaunchForSessionis the core session-aware connection dispatch function with complex branching (HTTP vs stdio, session pool hit/miss, double-check locking under mutex). It had several untested paths — specifically the double-check locking pattern and the startup timeout.EffectiveContainerRuntimeCommandwas at 0% because its private counterparteffectiveContainerRuntimeCommandwas tested directly, but the exported wrapper function was never called in tests.Tests Added
internal/launcher/getorlaunchforsession_test.goTestGetOrLaunchForSession_DoubleCheckLockHit: Tests the double-check locking pattern — simulates a concurrent goroutine pre-populating the session pool while another goroutine is blocked on the mutex, verifying the second goroutine returns the cached connection without launching a new processTestGetOrLaunchForSession_PoolStop_CleansDrained: Tests thatLauncher.Close()callsSessionConnectionPool.Stop()which removes all connections from the poolTestGetOrLaunchForSession_StartupTimeout: Tests thelaunchStdioConnectiontimeout path by usingsleep 60as the command with a 200ms startup timeout, covering thetime.Afterbranch in the select statementinternal/config/effective_container_runtime_command_test.goTestEffectiveContainerRuntimeCommand_PublicWrapper: Covers the exportedEffectiveContainerRuntimeCommandfunction with nil gateway, docker runtime, podman runtime, and explicit command path override — verifies agreement with the private implementationCoverage Report
Test Execution
All new tests pass:
TestGetOrLaunchForSession_DoubleCheckLockHit— PASSTestGetOrLaunchForSession_PoolStop_CleansDrained— PASSTestGetOrLaunchForSession_StartupTimeout— PASS (completes in ~200ms)TestEffectiveContainerRuntimeCommand_PublicWrapper— PASSGenerated by Test Coverage Improver
Warning
Firewall blocked 9 domains
The following domains were blocked by the firewall during workflow execution:
go.googlesource.comgo.opentelemetry.iogo.yaml.ingolang.orggoogle.golang.orggopkg.ingoproxy.ioproxy.golang.orgreleaseassets.githubusercontent.comSee Network Configuration for more information.