Improve config and security #36

thoreinstein · 2026-01-16T16:27:14Z

No description provided.

Introduce validation infrastructure and security best practices for configuration management: - Add ValidateMergeMethod() to validate GitHub merge methods - Add CheckSecurityWarnings() to detect tokens in config files - Add Config.Validate() method called during Load() - Comprehensive test coverage for validation and security checks - Add AIConfig fields for per-provider model defaults (preparation) This establishes the foundation for safer configuration handling and warns users when sensitive data is stored insecurely.

Enable per-repository configuration via .rig.toml with proper precedence: 1. Environment variables (RIG_*) 2. Repository-local config (.rig.toml in git root or current dir) 3. User config (~/.config/rig/config.toml) 4. Defaults Implement loadRepoLocalConfig() and findGitRoot() to discover and merge local config on top of user config. This allows teams to share repo-specific settings (default merge method, AI provider) while keeping secrets in env vars. Display security warnings at config load time to guide users toward environment variables for sensitive data.

Enable flexible AI provider configuration with provider-specific defaults: - Add AnthropicModel, GroqModel, OllamaModel, OllamaEndpoint to AIConfig - Update NewProvider() to use per-provider defaults when Model is empty - Allow global ai.model to override provider defaults when set - Maintain backward compatibility with existing configs Precedence for model selection: 1. Global ai.model (if set) 2. Provider-specific default (anthropic_model, groq_model, ollama_model) 3. Built-in fallback This allows users to configure different defaults per provider while keeping a single active provider setting.

Copilot

Pull request overview

This PR adds configuration validation, security warnings for sensitive tokens in config files, and improves AI provider model configuration by introducing per-provider default models.

Changes:

Added security warnings for tokens stored in config files instead of environment variables
Implemented merge method validation with new ValidateMergeMethod function and Config.Validate method
Enhanced AI configuration with per-provider default models (Anthropic, Groq, Ollama) and corresponding fallback logic
Added repository-local config support (.rig.toml) with precedence documentation
Added comprehensive test coverage for security warnings, merge method validation, and config validation

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
pkg/config/config_test.go	Adds comprehensive test coverage for security warnings, merge method validation, and config validation
pkg/config/config.go	Implements security warnings, merge method validation, per-provider AI model defaults, and config validation
pkg/ai/provider.go	Updates AI provider initialization to use per-provider default models when global model is empty
cmd/root.go	Adds repository-local config loading with precedence handling and security warnings display at startup
cmd/pr_merge.go	Updates merge method resolution to validate and return errors for invalid merge methods

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

pkg/config/config.go

cmd/root.go

cmd/pr_merge.go

Enforce merge method validation before attempting GitHub API calls: - Update resolveMergeMethod() to return error on invalid method - Call ValidateMergeMethod() to check against allowed values - Fail fast with clear error before workflow execution - Propagate validation errors to user This prevents GitHub API errors from invalid merge methods and provides immediate feedback when misconfigured.

Add comprehensive test coverage for .rig.toml loading: - findGitRoot(): 11 test cases covering regular repos, worktrees, subdirectories, and non-git directories - loadRepoLocalConfig(): 13 test cases covering config loading, cascading merges, malformed files, and verbose output - Config precedence: 3 integration tests verifying env > repo > user Uses Go 1.24's t.Chdir() for cleaner directory switching in tests. Coverage: findGitRoot 92%, loadRepoLocalConfig 90%.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-16T18:01:51Z

cmd/pr_merge.go

+	mergeMethod, err := resolveMergeMethod(cfg, prMergeMergeMethod)
+	if err != nil {
+		return err
+	}


The new validation logic in resolveMergeMethod lacks test coverage. The function now returns an error for invalid merge methods, but there's no pr_merge_test.go file to verify this behavior. Consider adding tests for: valid merge methods (merge, squash, rebase), invalid merge methods, empty values with config fallback, and empty values with default fallback.

Copilot · 2026-01-16T18:01:52Z

pkg/ai/provider.go

+		model := cfg.Model
+		if model == "" {
+			model = cfg.AnthropicModel
+		}


The new per-provider model fallback logic lacks test coverage. There are no tests verifying that when cfg.Model is empty, the provider-specific defaults (AnthropicModel, GroqModel, OllamaModel) are correctly used. Consider adding tests to pkg/ai/provider_test.go to cover these fallback scenarios for all three providers.

Copilot · 2026-01-16T18:01:52Z

pkg/config/config.go

+var ValidMergeMethods = []string{"merge", "squash", "rebase"}
+
+// ValidateMergeMethod validates that a merge method is supported.
+// Returns the method if valid, or an error if not.


The comment on line 179 is misleading. The function does not 'return the method' - it returns an error or nil. The comment should be updated to say 'Returns nil if valid, or an error if not.'

Suggested change

// Returns the method if valid, or an error if not.

// Returns nil if valid, or an error if not.

cmd/root.go

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-16T18:10:41Z

cmd/root.go

+	// Check for security warnings (tokens in config file)
+	cfg, err := config.Load()
+	if err != nil {
+		if verbose {
+			fmt.Fprintf(os.Stderr, "Warning: could not load config for security checks: %v\n", err)
+		}
+		return
+	}
+
+	warnings := config.CheckSecurityWarnings(cfg)
+	for _, w := range warnings {
+		fmt.Fprintf(os.Stderr, "Warning: %s\n", w.Message)
+	}
+}


When config.Load() fails in initConfig() (line 91), the function returns early and security warnings are never displayed. This means that if a user has both invalid configuration AND tokens in their config file, they'll only see the validation error but not the security warnings.

Consider either:

Running CheckSecurityWarnings before validation by loading the raw config without validation

Showing security warnings even when validation fails (they might be more urgent than validation issues)

Documenting that security warnings are only shown for valid configs

Copilot · 2026-01-16T18:10:42Z

cmd/root_test.go

+func TestFindGitRoot_GitWorktree(t *testing.T) {
+	tmpDir := evalSymlinks(t, t.TempDir())
+
+	// Create a worktree-style .git file (file, not directory)
+	// Git worktrees have a .git file with content: "gitdir: /path/to/main/repo/.git/worktrees/name"
+	gitFile := filepath.Join(tmpDir, ".git")
+	gitFileContent := "gitdir: /some/other/path/.git/worktrees/feature-branch"
+	if err := os.WriteFile(gitFile, []byte(gitFileContent), 0644); err != nil {
+		t.Fatalf("Failed to create .git file: %v", err)
+	}


The test creates worktree-style git configurations but uses hardcoded paths like "/some/other/path/.git/worktrees/feature-branch" in the gitdir content. While this is fine for testing the detection logic, consider adding a comment explaining that the actual path doesn't need to exist for these tests since the function only checks for the presence of the .git file, not its content validity.

Copilot · 2026-01-16T18:10:42Z

cmd/pr_merge.go

 // resolveMergeMethod determines the merge method to use.
-func resolveMergeMethod(cfg *config.Config, flagValue string) string {
-	if flagValue != "" {
-		return flagValue
+// Returns an error if the method is invalid.
+func resolveMergeMethod(cfg *config.Config, flagValue string) (string, error) {
+	method := flagValue
+	if method == "" {
+		method = cfg.GitHub.DefaultMergeMethod
 	}
-	if cfg.GitHub.DefaultMergeMethod != "" {
-		return cfg.GitHub.DefaultMergeMethod
+	if method == "" {
+		method = "squash" // Default fallback
 	}
-	return "squash" // Default fallback
+
+	// Validate the merge method
+	if err := config.ValidateMergeMethod(method); err != nil {
+		return "", err
+	}
+
+	return method, nil
 }


Missing test coverage for the resolveMergeMethod function in pr_merge.go. This function now returns an error and validates merge methods, but there are no tests to verify:

That valid methods are accepted

That invalid methods return errors

That the precedence chain (flag -> config -> default) works correctly

That validation errors are properly propagated

Consider adding test coverage similar to the tests added for ValidateMergeMethod in config_test.go.

Copilot · 2026-01-16T18:10:42Z

pkg/ai/provider.go

+		// Use global model if set, otherwise use provider-specific default
+		model := cfg.Model
+		if model == "" {
+			model = cfg.AnthropicModel
+		}
+		return NewAnthropicProvider(apiKey, model, logger), nil


The AIConfig now includes provider-specific model fields (AnthropicModel, GroqModel, OllamaModel). However, when a provider is selected and its specific model field is empty, there's no validation or fallback to ensure a model is set. For example, if a user sets:

provider = "anthropic"

model = "" (empty, so it should use provider default)

anthropic_model = "" (also empty)

The code will pass an empty string to NewAnthropicProvider. While NewAnthropicProvider has a fallback to anthropicDefaultModel (line 35-37 of anthropic.go), this creates an inconsistency where the fallback is split between config defaults and provider code.

Consider validating in the config that when Model is empty, the corresponding provider-specific model field must have a value, or document this behavior more clearly.

Copilot · 2026-01-16T18:10:42Z

pkg/config/config.go

 	viper.SetDefault("ai.enabled", true)
 	viper.SetDefault("ai.provider", "anthropic")
-	viper.SetDefault("ai.model", "claude-sonnet-4-20250514")
+	viper.SetDefault("ai.model", "") // Empty means use per-provider default


The default AI model is being changed from "claude-sonnet-4-20250514" to empty string. While the per-provider defaults maintain the same model, this changes the behavior: previously, if a user explicitly set Model to a value and later deleted it, they would get the default model. Now they need to understand the per-provider model system.

This is a behavioral change that could affect existing users. Consider:

Adding a migration note in documentation or changelog

Ensuring this is clearly documented in the configuration examples

Copilot · 2026-01-16T18:10:43Z

pkg/config/config_test.go

+	// Ensure all env vars are not set
+	t.Setenv("RIG_GITHUB_TOKEN", "")
+	t.Setenv("RIG_JIRA_TOKEN", "")
+	t.Setenv("JIRA_TOKEN", "")
+	t.Setenv("RIG_AI_API_KEY", "")
+	t.Setenv("ANTHROPIC_API_KEY", "")
+	t.Setenv("GROQ_API_KEY", "")
+


The test sets multiple environment variables to empty strings ("") using t.Setenv. However, setting an environment variable to an empty string is different from unsetting it. The CheckSecurityWarnings function checks os.Getenv("RIG_GITHUB_TOKEN") == "", which will be true for both unset and empty string values, but this is potentially fragile.

Consider using t.Setenv to actually unset the variables or documenting why empty strings are sufficient for these tests.

Copilot · 2026-01-16T18:10:43Z

pkg/ai/provider.go

 	case ProviderOllama:
-		return NewOllamaProvider(cfg.Endpoint, cfg.Model, logger), nil
+		// Use global model if set, otherwise use provider-specific default
+		model := cfg.Model
+		if model == "" {
+			model = cfg.OllamaModel
+		}
+		// Use global endpoint if set, otherwise use provider-specific default
+		endpoint := cfg.Endpoint
+		if endpoint == "" {
+			endpoint = cfg.OllamaEndpoint
+		}
+		return NewOllamaProvider(endpoint, model, logger), nil


Inconsistent model fallback behavior between providers. For Ollama, both the endpoint and model have provider-specific defaults. However, for Anthropic and Groq, there's no provider-specific endpoint field even though custom endpoints might be useful for local proxies or alternative implementations.

This asymmetry could be confusing. Consider either:

Adding provider-specific endpoint fields for all providers (AnthropicEndpoint, GroqEndpoint) for consistency

Documenting why Ollama is treated differently (likely because it's commonly run locally)

Copilot · 2026-01-16T18:10:43Z

pkg/config/config.go

+	// Validate configuration
+	if err := config.Validate(); err != nil {
+		return nil, errors.Wrap(err, "config validation failed")
+	}
+
 	return config, nil
 }


Missing test coverage for the validation that occurs during config.Load(). The existing tests (TestLoad_WithDefaults and TestLoad_WithConfigFile) don't verify that invalid merge methods cause Load() to fail with a validation error.

Consider adding a test case that creates a config with an invalid merge method and verifies that config.Load() returns a validation error. This would ensure the validation integration is working correctly.

thoreinstein added 3 commits January 16, 2026 11:11

Copilot AI review requested due to automatic review settings January 16, 2026 16:27

Copilot started reviewing on behalf of thoreinstein January 16, 2026 16:27 View session

Copilot AI reviewed Jan 16, 2026

View reviewed changes

pkg/config/config.go Show resolved Hide resolved

cmd/root.go Show resolved Hide resolved

cmd/pr_merge.go Show resolved Hide resolved

thoreinstein force-pushed the rig-pth branch from 234b6ad to 92b4095 Compare January 16, 2026 17:17

Copilot AI review requested due to automatic review settings January 16, 2026 17:59

Copilot started reviewing on behalf of thoreinstein January 16, 2026 17:59 View session

Copilot AI reviewed Jan 16, 2026

View reviewed changes

Update cmd/root.go

db45e56

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings January 16, 2026 18:05

Copilot started reviewing on behalf of thoreinstein January 16, 2026 18:06 View session

Copilot AI reviewed Jan 16, 2026

View reviewed changes

thoreinstein merged commit b713ec9 into main Jan 16, 2026
11 checks passed

thoreinstein deleted the rig-pth branch January 16, 2026 21:01

	// Returns the method if valid, or an error if not.
	// Returns nil if valid, or an error if not.

Improve config and security #36

Improve config and security #36

Uh oh!

Conversation

thoreinstein commented Jan 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants