Conversation
ExpandEnvArgs is a generic env-var expansion utility used to expand Docker -e flags. It lives in internal/config/docker_helpers.go but has only one non-test caller (internal/mcp/connection.go) and does not belong to config's domain. Move it to internal/envutil/ alongside GetEnvString, GetEnvInt, GetEnvDuration, and GetEnvBool, which is a better semantic fit. This removes the mcp → config import dependency for a non-config concern. Changes: - Add internal/envutil/expand_env_args.go with the function and its own logExpand logger (namespace: envutil:expand) - Remove ExpandEnvArgs from internal/config/docker_helpers.go - Update internal/mcp/connection.go to import envutil instead of config - Update internal/mcp/connection_test.go to use envutil.ExpandEnvArgs - Move ExpandEnvArgs tests to internal/envutil/expand_env_args_test.go Addresses the quick-win refactoring opportunity identified in #3211. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
lpcox
deleted the
repo-assist/improve-move-ExpandEnvArgs-to-envutil-2026-04-05-0490e46850a1cc51
branch
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors a generic Docker -e VAR_NAME environment expansion helper (ExpandEnvArgs) out of internal/config into internal/envutil, removing an unnecessary mcp → config dependency and co-locating it with other env utilities.
Changes:
- Moved
ExpandEnvArgsimplementation frominternal/config/docker_helpers.gotointernal/envutil/expand_env_args.go. - Migrated the associated unit tests from
internal/configtointernal/envutil. - Updated MCP connection code/tests to call
envutil.ExpandEnvArgsinstead ofconfig.ExpandEnvArgs.
Show a summary per file
| File | Description |
|---|---|
| internal/envutil/expand_env_args.go | New home for ExpandEnvArgs (with its own debug logger namespace). |
| internal/envutil/expand_env_args_test.go | New test file covering ExpandEnvArgs behavior in envutil. |
| internal/config/docker_helpers.go | Removes ExpandEnvArgs from config/Docker helpers. |
| internal/config/docker_helpers_and_env_test.go | Removes the old ExpandEnvArgs tests from config. |
| internal/mcp/connection.go | Switches call site to envutil.ExpandEnvArgs, dropping config import. |
| internal/mcp/connection_test.go | Updates the existing MCP-level test to use envutil.ExpandEnvArgs. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 1
Comment on lines
254
to
+262
| require.NoError(t, os.Setenv(k, v)) | ||
| } | ||
| t.Cleanup(func() { | ||
| for k := range tt.envVars { | ||
| os.Unsetenv(k) | ||
| } | ||
| }) | ||
|
|
||
| result := config.ExpandEnvArgs(tt.args) | ||
| result := envutil.ExpandEnvArgs(tt.args) |
There was a problem hiding this comment.
In this test, environment variables are managed with os.Setenv + manual t.Cleanup. Since this file is already using Go’s testing.T helpers elsewhere, consider switching to t.Setenv here as well to simplify the code and avoid accidental env leakage if the test suite is ever parallelized.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 This is an automated pull request from Repo Assist.
Summary
Moves
ExpandEnvArgsfrominternal/config/docker_helpers.goto a newinternal/envutil/expand_env_args.go, co-locating it with the other env-var utilities (GetEnvString,GetEnvInt,GetEnvDuration,GetEnvBool).Root Cause / Motivation
ExpandEnvArgsis a generic utility that expands Docker-e VAR_NAMEflags by reading from the process environment. Despite living in theconfigpackage, it:internal/mcp/connection.go, creating anmcp → configimport dependency purely for this one functionGetEnvString/GetEnvBoolalready inenvutilThis is the "quick win" refactoring identified in issue #3211 (Semantic Function Clustering analysis).
Changes
internal/envutil/expand_env_args.goExpandEnvArgswithlogExpandlogger (envutil:expand)internal/envutil/expand_env_args_test.goconfigpackageinternal/config/docker_helpers.goExpandEnvArgsfunction (30 lines)internal/config/docker_helpers_and_env_test.goTestExpandEnvArgs*tests (144 lines)internal/mcp/connection.goenvutilinstead ofconfiginternal/mcp/connection_test.goenvutil.ExpandEnvArgsinstead ofconfig.ExpandEnvArgsTest Status
Note: The sandboxed CI environment during Repo Assist runs does not have network access to download Go module dependencies, so a full
make buildcould not be completed locally. However:gofmt -econfirms all changed files parse correctly (no syntax errors)GitHub Actions CI will run the full build and test suite.
Closes nothing directly; addresses #3211 quick-win checklist item.