Skip to content

refactor: replace test-overridden globals with parallel-safe injection - #3332

Merged
dgageot merged 6 commits into
docker:mainfrom
dgageot:worktree-board-0b5e3cf3b4804cd2
Jun 30, 2026
Merged

dgageot merged 6 commits into
docker:mainfrom
dgageot:worktree-board-0b5e3cf3b4804cd2

Conversation

@dgageot

@dgageot dgageot commented Jun 30, 2026

Copy link
Copy Markdown
Member

What

Removes a cluster of mutable package-level global variables that tests were overriding in place. Overriding a shared global forces the tests that do it to run serially; replacing each global with either an explicit parameter or a field on the owning struct lets those tests run with t.Parallel() and no shared-state mutation.

This is a behavior-preserving refactor — production defaults are unchanged. Each commit is independently green (build, go test -race, golangci-lint, and the repo's custom cops).

Why

Several packages carried var x = … globals whose only reason for being a var (not a const or a struct field) was so a test could reassign them via orig := x; x = …; t.Cleanup(func(){ x = orig }). That pattern:

  • is not race-safe and silently blocks t.Parallel() on the affected tests;
  • spreads a half-dozen slightly different idioms for the same need;
  • leaves test-only seams (and the occasional exported Reset*ForTest) in production code.

How

Two consistent patterns, matching what the codebase already does well elsewhere (pkg/userid.Resolver, pkg/paths):

  1. Explicit parameter when the consumer is a free function.
  2. Field on the owning struct + a zero-value-means-default accessor when the consumer is a method, so existing struct literals (including hand-built test ones) keep working untouched.

Commits

Commit Package Global removed Pattern Tests unblocked
refactor(toolinstall) pkg/toolinstall 4 × max* limit vars limits value type + Registry.extractor() accessor 3
refactor(runtime) pkg/runtime defaultStreamIdleTimeout const default + handleStream parameter 2
refactor(mcp) pkg/tools/mcp unmanagedOAuthWaitTimeout const default + per-oauthTransport field 1
refactor(notification) pkg/tui/components/notification timerDuration Manager.autoHideDuration field, defaulted in New() 6
refactor(tui) pkg/tui exitFunc, shutdownTimeout per-appModel fields + accessors 10
refactor(cmd/root) cmd/root loadUserConfig userConfigLoader parameter; tests inject MapEnvProvider gateway/default-model tests

~22 tests that previously had to run serially now run in parallel.

Safety notes

  • The zip/tar-bomb extraction bounds, the OAuth wait timeout, and the stream idle timeout all keep their exact production defaults.
  • The "zero-value-means-default" accessors were checked against every production construction site: Registry is only built via NewRegistry() and Manager only via New(), so the default is always applied in production. The zero-value fallback only ever benefits test literals.

Test plan

  • task build
  • go test -race ./... for each touched package
  • task lint (golangci-lint + 18 custom cops) — clean

@dgageot
dgageot requested a review from a team as a code owner June 30, 2026 15:37

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟢 APPROVE

This is a clean, well-structured refactor that replaces mutable package-level globals with injected parameters and struct fields, enabling parallel test execution without any behavioral regressions.

What was checked:

  • pkg/toolinstall/registry.go — extractor() zero-value-means-default accessor: the limits{} zero check correctly distinguishes "unset" from "configured", and the comment explicitly documents the design. No test constructs partial limits, so the theoretical edge case does not apply. ✅
  • pkg/tui/components/notification/notification.go — Manager.autoHideDuration zero-value for tests: the comment documents that tests intentionally use zero duration for prompt timer resolution, while all production code goes through New() which sets defaultDuration. ✅
  • pkg/runtime/streaming.go — defaultStreamIdleTimeout replaced with const + parameter: production defaults preserved. ✅
  • pkg/tools/mcp/oauth.go — unmanagedOAuthWaitTimeout moved to oauthTransport field with zero-value accessor: production default preserved. ✅
  • pkg/tui/tui.go — exitFunc and shutdownTimeout moved to appModel fields with accessor defaults: production behavior unchanged. ✅
  • cmd/root/flags.go / cmd/root/models.go — loadUserConfig replaced with injected userConfigLoader: production path still calls the real loader. ✅

No bugs were introduced by this PR.

@aheritier aheritier added area/cli CLI commands, flags, output formatting area/mcp MCP protocol, MCP tool servers, integration area/runtime Runtime engine, agent loop execution, tool dispatch, loop detection area/tools For features/issues/fixes related to the usage of built-in and MCP tools area/tui For features/issues/fixes related to the TUI kind/refactor PR refactors code without behavior change labels Jun 30, 2026
dgageot added 6 commits June 30, 2026 18:07
Replace the mutable package-level defaultStreamIdleTimeout var (which
tests overrode in place, forcing them to run serially) with a const
default threaded through handleStream as an explicit parameter.

Production passes defaultStreamIdleTimeout; tests pass their own value,
so the idle-timeout and context-cancellation tests now run with
t.Parallel() and no shared-state mutation.
Replace the mutable package-level unmanagedOAuthWaitTimeout var (which a
test overrode in place, forcing it to run serially) with a const default
plus an optional per-transport waitTimeout field resolved via
waitTimeoutOrDefault (zero means default).

Production transports leave the field unset; the timeout test sets a
short value on its own transport, so it now runs with t.Parallel() and
no shared-state mutation.
Replace the package-level timerDuration var (which tests overrode in
place via a t.Cleanup closure, forcing them to run serially) with an
autoHideDuration field on Manager, defaulted by New().

Tests set a zero duration on their own Manager so the auto-hide timer
resolves promptly, and the six affected tests now run with t.Parallel()
and no shared-state mutation.
Replace the mutable package-level exitFunc and shutdownTimeout vars
(which tests overrode in place, forcing them to run serially) with
per-appModel fields resolved through exitFn()/shutdownTimeoutOrDefault()
(zero means the package default os.Exit / 5s).

Tests set these on their own model, so the ten exit/cleanup/ctrl-c tests
now run with t.Parallel() and no shared-state mutation.
…obal

Replace the mutable package-level loadUserConfig var with an explicit
userConfigLoader parameter on addGatewayFlags. Production callers pass
userconfig.Load; the gateway/default-model tests inject a stub.

The gateway and default-model tests additionally route env through an
injected environment.MapEnvProvider instead of t.Setenv, so they now run
fully in parallel with no shared-state mutation. The models tests, which
still rely on credential env vars, point paths.SetConfigDir at an empty
temp dir to get an empty user config without the global.
@dgageot
dgageot force-pushed the worktree-board-0b5e3cf3b4804cd2 branch from 833cf87 to 61c2f6f Compare June 30, 2026 16:11
@dgageot
dgageot merged commit 540b180 into docker:main Jun 30, 2026
7 checks passed
social4hyq pushed a commit to social4hyq/homebrew-core that referenced this pull request Sep 20, 2026
docker-agent 1.93.0

Created-by: HarmonybrewBot
Commit-by: HarmonybrewBot
Merged-by: HarmonybrewBot
Description: Created by `brew bump`

---

Created with `brew bump-formula-pr`.<details>
  <summary>release notes</summary>
  <pre>This release adds OpenRouter as a built-in provider, introduces per-model gateway bypass support, and allows a dedicated model for session compaction, alongside several internal refactoring improvements.

## What's New
- Adds OpenRouter as a first-class built-in provider; when `OPENROUTER_API_KEY` is set, the provider is auto-detected and its base URL resolved automatically
- Adds a `compaction_model` field to allow a dedicated model for session summary generation, separate from the primary agent model
- Adds a `bypass_models_gateway` boolean field on a per-model basis, allowing specific models to connect directly to their provider instead of routing through the configured models gateway

## Technical Changes
- Replaces mutex usage with atomic types (`atomic.Bool`, `atomic.Pointer`) for single mutable fields in several structs
- Replaces package-level global variables with explicit parameters or struct fields to enable parallel-safe testing across several packages (`toolinstall`, `runtime`, `mcp`, `notification`, `tui`, `cmd/root`)
- Adds a concurrency group to the PR review trigger CI workflow to prevent duplicate reviews for the same PR
---

## What's Changed
* docs: update CHANGELOG.md for v1.92.0 by @docker-read-write[bot] in docker/docker-agent#3331
* refactor: replace single-variable mutexes with atomic types by @dgageot in docker/docker-agent#3330
* ci: add concurrency group to pr-review-trigger to prevent duplicate reviews by @derekmisler in docker/docker-agent#3334
* refactor: replace test-overridden globals with parallel-safe injection by @dgageot in docker/docker-agent#3332
* feat(compaction): allow a dedicated model for session summary generation by @Sayt-0 in docker/docker-agent#3335
* feat: add OpenRouter provider by @dgageot in docker/docker-agent#3337
* feat: add per-model bypass_models_gateway option by @dgageot in docker/docker-agent#3338


**Full Changelog**: docker/docker-agent@v1.92.0...v1.93.0
</pre>
  <p>View the full release notes at <a href="https://github.com/docker/docker-agent/releases/tag/v1.93.0">https://github.com/docker/docker-agent/releases/tag/v1.93.0</a>.</p>
</details>
<hr>

See merge request: Harmonybrew/homebrew-core!13152
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/cli CLI commands, flags, output formatting area/mcp MCP protocol, MCP tool servers, integration area/runtime Runtime engine, agent loop execution, tool dispatch, loop detection area/tools For features/issues/fixes related to the usage of built-in and MCP tools area/tui For features/issues/fixes related to the TUI kind/refactor PR refactors code without behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants