Skip to content

refactor: replace test-mutated globals with dependency injection#3326

Merged
Sayt-0 merged 3 commits into
docker:mainfrom
dgageot:worktree-board-e8667c0ab5517be5
Jun 30, 2026
Merged

refactor: replace test-mutated globals with dependency injection#3326
Sayt-0 merged 3 commits into
docker:mainfrom
dgageot:worktree-board-e8667c0ab5517be5

Conversation

@dgageot

@dgageot dgageot commented Jun 30, 2026

Copy link
Copy Markdown
Member

What

Replaces several package-level globals that tests mutated at runtime with proper dependency injection. Tests no longer swap shared globals, so they can run with t.Parallel() safely and there is no hidden cross-test coupling.

Why

We had a recurring pattern where production code reached for a package-level singleton (an HTTP client, a fetch function, a clock, a catalog loader) and tests overrode it via var x = ...; x = stub; t.Cleanup(...) or a dedicated SetXForTest helper. That pattern forces sequential tests, is racy under -race if a goroutine ever reads the global concurrently, and exercises a wiring that only exists in tests. Injecting the dependency removes the global entirely.

Changes

Five globals converted (production behavior unchanged — the default still wires the original implementation):

  • pkg/modelsdev — removed var fetchCatalog; added a Store.fetch field + WithFetcher(...) option. loadDatabase takes a fetcher parameter. Tests inject a stub fetcher.
  • pkg/toolinstall — removed var doInstallFn; resolve/safeInstall take an installFunc, with EnsureCommand supplying the real doInstall.
  • pkg/hooks/builtins — removed var httpPostClient and SetHTTPPostClientUnsafeForTest; Register now takes functional Options (WithHTTPPostClient(...)), and http_post is a closure over an httpDoer. The SSRF-safe client remains the production default; only tests inject the unsafe client. Deleted export_test.go and main_test.go.
  • pkg/tui/components/reasoningblock — replaced package-level var nowFunc with a per-instance Model.now clock field defaulting to time.Now.
  • pkg/gateway — introduced a Loader type (memoization via sync.Once + injectable fetch), NewLoader()/NewStaticLoader(), and context plumbing WithLoader/loaderFrom with a package-level defaultLoader. Removed the catalogOnce/catalogCached/catalogOverride globals and deleted testing.go. Production call sites use the default loader via context transparently.

A follow-up commit hardens the new seams: nil-safe WithLoader/loaderFrom, a zero-value-safe Loader.load and Store.fetcher(), and a corrected godoc placement — all defensive, no behavior change.

Validation

  • task lint — golangci-lint, custom cops, and go mod tidy all clean
  • task test — full suite passes
  • go test -race on the affected packages passes; the converted tests now run t.Parallel()

@dgageot dgageot requested a review from a team as a code owner June 30, 2026 14:18

@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

The refactor cleanly converts five package-level globals to dependency injection across pkg/gateway, pkg/toolinstall, pkg/hooks/builtins, pkg/tui/components/reasoningblock, and pkg/modelsdev. Production behaviour is preserved — every call site wires the original implementation as the default.

What was reviewed:

  • Nil-safety of new zero-value code paths (Store.fetcher(), Loader.load, loaderFrom)
  • Concurrency correctness of sync.Once memoization in the new gateway.Loader
  • Singleflight cross-test contamination via the residual package-level installGroup
  • Data-race exposure on Model.now in reasoningblock
  • Default wiring correctness in all five refactored packages

Findings: All hypotheses examined were either non-issues (code paths that can never be reached) or correctly scoped (e.g., TestResolve_ConcurrentPanic_DoesNotCrash does not call t.Parallel(), so the shared installGroup cannot cause cross-test contamination in a standard go test ./... run). No CONFIRMED or LIKELY bugs were found in the introduced code.

@Sayt-0 Sayt-0 merged commit cda2ec5 into docker:main Jun 30, 2026
10 checks passed
@aheritier aheritier added area/gateway Gateway, proxy, and routing 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/gateway Gateway, proxy, and routing 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