refactor: remove config global state#51
Open
dgerlanc wants to merge 15 commits into
Open
Conversation
Covers the plan to replace the config singleton pattern with a pure constructor + closure-based wiring, enabling parallel tests and explicit dependency passing.
Fix signature mismatches (GetConfigDir returns error, logger.Init takes Options struct, audit.Init signature), address logAudit dependency on config globals, add completion.go restructuring, cover benchmark_test.go and fuzz_test.go, preserve io.Reader parameter on ProcessWithResult, and clarify root command Run wiring.
Resolve logAudit design (pass cfgPath/cfgErr through ProcessWithResult), add Process() wrapper signature, inline runHook into root Run closure, fix audit.Init to be called unconditionally, note IsVerbose/IsDryRun removal, add EnsureConfigFiles/GetDefaultConfig to unchanged list, address cmd test rewrites for package-level vars, and add mmi init first-run note.
Return loadEmbeddedDefaults() instead of nil on parse errors to match current Init() behavior and prevent nil pointer dereferences. Keep Execute() as the public entry point wrapping buildRootCmd().
Ensures Load() never returns nil config, matching current Init() behavior where callers always get a usable (deny-all) config.
10 tasks covering config.Load(), hook signature changes, cmd builder pattern, and comprehensive test updates across all packages.
Address validate missing-config behavioral change, note intermediate commit ordering, fix init.go configureClaudeSettings/getClaudeSettingsPath param threading, expand hook_test.go coverage, enumerate validate_test and init_test direct function calls.
Remove global variables (globalConfig, configInitialized, globalInitError, globalConfigPath) and functions (Init, Get, InitError, GetConfigPath, Reset) from the config package. Replace with a pure Load() function that returns (*Config, string, error) and always provides a non-nil Config with embedded defaults on any error path.
Replace SetupTestConfig (which used global config state) with LoadTestConfig that parses TOML and returns a *Config for testing.
- Rewrite all cmd tests to use buildRootCmd() with SetArgs/SetIn/SetOut - Update root.go Run to use cmd.InOrStdin/OutOrStdout/ErrOrStderr - Update validate.go to use cmd.OutOrStdout for testability - Remove all global state references (resetGlobalState, initForce, etc.)
- Remove TestMain that set up global config state - Add loadTestConfig helper using config.LoadConfig directly - Update TestProcess to pass cfg explicitly to hook.Process - Update TestStripWrappers, TestCheckSafe, TestCheckSafeUnsafe - Update TestInitConfig and TestConfigCustomization to use config.Load - Update runMmi integration helper to create its own temp config - Update benchmarks and fuzz tests for explicit config parameter
- Replace setupTestConfig with loadTestConfig returning (*Config, string) - Update all ProcessWithResult calls to pass cfg, cfgPath, cfgErr - Replace config.Reset/Init patterns with config.Load or empty Config - Update audit config path/error tests to use config.Load
Add t.Parallel() to tests that don't rely on global state (t.Setenv or audit.Init/Close/Reset). Tests using the global audit logger in hook_test.go are excluded since audit.Init is not safe for concurrent use.
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
1 participant
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.
Summary
config.Init()/config.Get()/Reset()singleton pattern with a pureconfig.Load()constructor that returns(*Config, string, error)hook.ProcessWithResultandcmdlayer via function parameters and closurescmdpackageinit()functions and global flags with abuildRootCmd()builder patternt.Parallel()on 43 tests now that global config state is eliminatedTest plan
go test ./... -race -count=1passes (all 7 packages)go vet ./...cleanconfig.Get,config.Init,config.Reset,config.InitError,config.GetConfigPath)globalConfig,configInitialized,globalInitError,globalConfigPath)