From 1c833e74b976a2c68fe5ba9504c9e3c997271076 Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Tue, 24 Mar 2026 11:53:29 -0400 Subject: [PATCH] refactor(ci): consolidate config precedence helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move CI review and GitHub Actions workflow config resolution through shared helpers in internal/config. Verification: - roborev fix --open --list - go fmt ./... - go vet ./... - go test ./... 🤖 Generated with [OpenAI Codex](https://openai.com/codex) Co-authored-by: OpenAI Codex --- cmd/roborev/ci.go | 124 ++-------------------- cmd/roborev/ci_test.go | 78 ++++++++++++-- cmd/roborev/ghaction.go | 19 +--- cmd/roborev/ghaction_test.go | 70 +++++++++++++ internal/config/config.go | 196 +++++++++++++++++++++++++++++++++++ 5 files changed, 345 insertions(+), 142 deletions(-) diff --git a/cmd/roborev/ci.go b/cmd/roborev/ci.go index 6cf81d6f1..45c99cb07 100644 --- a/cmd/roborev/ci.go +++ b/cmd/roborev/ci.go @@ -162,7 +162,7 @@ func runCIReview(ctx context.Context, opts ciReviewOpts) error { } // Resolve agents - agents := resolveAgentList( + agents := config.ResolveCIAgents( opts.agents, repoCfg, globalCfg) if len(agents) == 0 { return fmt.Errorf("no agents configured " + @@ -170,7 +170,7 @@ func runCIReview(ctx context.Context, opts ciReviewOpts) error { } // Resolve review types - reviewTypes := resolveReviewTypes( + reviewTypes := config.ResolveCIReviewTypes( opts.reviewTypes, repoCfg, globalCfg) if len(reviewTypes) == 0 { return fmt.Errorf("no review types configured " + @@ -183,21 +183,21 @@ func runCIReview(ctx context.Context, opts ciReviewOpts) error { } // Resolve reasoning - reasoningLevel, err := resolveCIReasoning( + reasoningLevel, err := config.ResolveCIReasoning( opts.reasoning, repoCfg, globalCfg) if err != nil { return err } // Resolve min severity - minSev, err := resolveCIMinSeverity( + minSev, err := config.ResolveCIMinSeverity( opts.minSeverity, repoCfg, globalCfg) if err != nil { return err } // Resolve synthesis agent - synthAgent := resolveCISynthesisAgent( + synthAgent := config.ResolveCISynthesisAgent( opts.synthesisAgent, repoCfg, globalCfg) log.Printf( @@ -264,7 +264,7 @@ func runCIReview(ctx context.Context, opts ciReviewOpts) error { prNumber = detected } - upsert := resolveCIUpsertComments( + upsert := config.ResolveCIUpsertComments( repoCfg, globalCfg) if err := postCIComment( ctx, ghRepo, prNumber, comment, upsert, @@ -285,118 +285,6 @@ func runCIReview(ctx context.Context, opts ciReviewOpts) error { return nil } -func resolveAgentList( - flag string, - repoCfg *config.RepoConfig, - globalCfg *config.Config, -) []string { - if flag != "" { - return splitTrimmed(flag) - } - if repoCfg != nil && len(repoCfg.CI.Agents) > 0 { - return repoCfg.CI.Agents - } - if globalCfg != nil && len(globalCfg.CI.Agents) > 0 { - return globalCfg.CI.Agents - } - // Default: empty string = auto-detect - return []string{""} -} - -func resolveReviewTypes( - flag string, - repoCfg *config.RepoConfig, - globalCfg *config.Config, -) []string { - if flag != "" { - return splitTrimmed(flag) - } - if repoCfg != nil && len(repoCfg.CI.ReviewTypes) > 0 { - return repoCfg.CI.ReviewTypes - } - if globalCfg != nil && len(globalCfg.CI.ReviewTypes) > 0 { - return globalCfg.CI.ReviewTypes - } - return []string{config.ReviewTypeSecurity} -} - -func resolveCIReasoning( - flag string, - repoCfg *config.RepoConfig, - globalCfg *config.Config, -) (string, error) { - if flag != "" { - n, err := config.NormalizeReasoning(flag) - if err != nil { - return "", err - } - return n, nil - } - if repoCfg != nil && repoCfg.CI.Reasoning != "" { - if n, err := config.NormalizeReasoning( - repoCfg.CI.Reasoning); err == nil { - return n, nil - } - } - return "thorough", nil -} - -func resolveCIMinSeverity( - flag string, - repoCfg *config.RepoConfig, - globalCfg *config.Config, -) (string, error) { - if flag != "" { - n, err := config.NormalizeMinSeverity(flag) - if err != nil { - return "", err - } - return n, nil - } - if repoCfg != nil && repoCfg.CI.MinSeverity != "" { - if n, err := config.NormalizeMinSeverity( - repoCfg.CI.MinSeverity); err == nil { - return n, nil - } - } - if globalCfg != nil && globalCfg.CI.MinSeverity != "" { - if n, err := config.NormalizeMinSeverity( - globalCfg.CI.MinSeverity); err == nil { - return n, nil - } - } - return "", nil -} - -func resolveCISynthesisAgent( - flag string, - repoCfg *config.RepoConfig, - globalCfg *config.Config, -) string { - if flag != "" { - return flag - } - if globalCfg != nil && globalCfg.CI.SynthesisAgent != "" { - return globalCfg.CI.SynthesisAgent - } - return "" -} - -// resolveCIUpsertComments determines whether to upsert PR comments. -// Priority: repo config > global config > false. -func resolveCIUpsertComments( - repoCfg *config.RepoConfig, - globalCfg *config.Config, -) bool { - if repoCfg != nil && repoCfg.CI.UpsertComments != nil { - return *repoCfg.CI.UpsertComments - } - if globalCfg != nil { - return globalCfg.CI.UpsertComments - } - return false -} - func splitTrimmed(s string) []string { parts := strings.Split(s, ",") out := make([]string, 0, len(parts)) diff --git a/cmd/roborev/ci_test.go b/cmd/roborev/ci_test.go index 42e711811..4f4eec817 100644 --- a/cmd/roborev/ci_test.go +++ b/cmd/roborev/ci_test.go @@ -159,7 +159,7 @@ func TestExtractHeadSHA(t *testing.T) { func TestResolveAgentList(t *testing.T) { t.Run("flag", func(t *testing.T) { - agents := resolveAgentList( + agents := config.ResolveCIAgents( "codex,gemini", nil, nil) assert.False(t, len(agents) != 2 || agents[0] != "codex" || @@ -167,33 +167,33 @@ func TestResolveAgentList(t *testing.T) { }) t.Run("default", func(t *testing.T) { - agents := resolveAgentList("", nil, nil) + agents := config.ResolveCIAgents("", nil, nil) assert.False(t, len(agents) != 1 || agents[0] != "") }) } func TestResolveReviewTypes(t *testing.T) { t.Run("flag", func(t *testing.T) { - types := resolveReviewTypes( + types := config.ResolveCIReviewTypes( "security,design", nil, nil) assert.Len(t, types, 2) }) t.Run("default", func(t *testing.T) { - types := resolveReviewTypes("", nil, nil) + types := config.ResolveCIReviewTypes("", nil, nil) assert.False(t, len(types) != 1 || types[0] != "security") }) } func TestResolveAgentList_EmptyFlag(t *testing.T) { // Comma-only flag should resolve to empty list. - agents := resolveAgentList(",", nil, nil) + agents := config.ResolveCIAgents(",", nil, nil) assert.Empty(t, agents) } func TestResolveReviewTypes_EmptyFlag(t *testing.T) { // Whitespace-comma flag should resolve to empty list. - types := resolveReviewTypes(" , ", nil, nil) + types := config.ResolveCIReviewTypes(" , ", nil, nil) assert.Empty(t, types) } @@ -249,12 +249,76 @@ func TestResolveCIUpsertComments(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := resolveCIUpsertComments(tt.repo, tt.global) + got := config.ResolveCIUpsertComments(tt.repo, tt.global) assert.Equal(t, tt.want, got) }) } } +func TestResolveCIReasoning(t *testing.T) { + t.Run("explicit flag wins", func(t *testing.T) { + got, err := config.ResolveCIReasoning("high", nil, nil) + require.NoError(t, err) + assert.Equal(t, "thorough", got) + }) + + t.Run("repo override wins", func(t *testing.T) { + got, err := config.ResolveCIReasoning("", &config.RepoConfig{ + CI: config.RepoCIConfig{Reasoning: "medium"}, + }, &config.Config{}) + require.NoError(t, err) + assert.Equal(t, "medium", got) + }) + + t.Run("invalid repo config falls back to default", func(t *testing.T) { + got, err := config.ResolveCIReasoning("", &config.RepoConfig{ + CI: config.RepoCIConfig{Reasoning: "nope"}, + }, nil) + require.NoError(t, err) + assert.Equal(t, "thorough", got) + }) +} + +func TestResolveCIMinSeverity(t *testing.T) { + t.Run("explicit flag wins", func(t *testing.T) { + got, err := config.ResolveCIMinSeverity("HIGH", nil, nil) + require.NoError(t, err) + assert.Equal(t, "high", got) + }) + + t.Run("repo override beats global", func(t *testing.T) { + got, err := config.ResolveCIMinSeverity("", &config.RepoConfig{ + CI: config.RepoCIConfig{MinSeverity: "medium"}, + }, &config.Config{ + CI: config.CIConfig{MinSeverity: "high"}, + }) + require.NoError(t, err) + assert.Equal(t, "medium", got) + }) + + t.Run("invalid repo config falls through to valid global", func(t *testing.T) { + got, err := config.ResolveCIMinSeverity("", &config.RepoConfig{ + CI: config.RepoCIConfig{MinSeverity: "bogus"}, + }, &config.Config{ + CI: config.CIConfig{MinSeverity: "critical"}, + }) + require.NoError(t, err) + assert.Equal(t, "critical", got) + }) +} + +func TestResolveCISynthesisAgent(t *testing.T) { + got := config.ResolveCISynthesisAgent("", &config.RepoConfig{}, &config.Config{ + CI: config.CIConfig{SynthesisAgent: "gemini"}, + }) + assert.Equal(t, "gemini", got) + + got = config.ResolveCISynthesisAgent("codex", nil, &config.Config{ + CI: config.CIConfig{SynthesisAgent: "gemini"}, + }) + assert.Equal(t, "codex", got) +} + func TestSplitTrimmed(t *testing.T) { tests := []struct { in string diff --git a/cmd/roborev/ghaction.go b/cmd/roborev/ghaction.go index b1434fb46..18feec359 100644 --- a/cmd/roborev/ghaction.go +++ b/cmd/roborev/ghaction.go @@ -119,23 +119,8 @@ func resolveWorkflowConfig( globalCfg, _ := config.LoadGlobal() repoCfg, _ := config.LoadRepoConfig(repoRoot) - - // Resolve agents: flag > repo [ci] agents > repo agent - // > global [ci] agents > global default_agent > default - if agentFlag != "" { - cfg.Agents = splitTrimmed(agentFlag) - } else if repoCfg != nil && - len(repoCfg.CI.Agents) > 0 { - cfg.Agents = repoCfg.CI.Agents - } else if repoCfg != nil && repoCfg.Agent != "" { - cfg.Agents = []string{repoCfg.Agent} - } else if globalCfg != nil && - len(globalCfg.CI.Agents) > 0 { - cfg.Agents = globalCfg.CI.Agents - } else if globalCfg != nil && - globalCfg.DefaultAgent != "" { - cfg.Agents = []string{globalCfg.DefaultAgent} - } + cfg.Agents = config.ResolveCIWorkflowAgents( + agentFlag, repoCfg, globalCfg) if roborevVersion != "" { cfg.RoborevVersion = roborevVersion diff --git a/cmd/roborev/ghaction_test.go b/cmd/roborev/ghaction_test.go index 0e117c64f..5afbcd1e8 100644 --- a/cmd/roborev/ghaction_test.go +++ b/cmd/roborev/ghaction_test.go @@ -6,6 +6,7 @@ import ( "runtime" "testing" + "github.com/roborev-dev/roborev/internal/config" "github.com/roborev-dev/roborev/internal/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -174,3 +175,72 @@ func TestGhActionCmd_NotGitRepo(t *testing.T) { require.Error(t, err, "expected error outside git repo") require.ErrorContains(t, err, "not a git repository", "expected 'not a git repository' error, got: %v", err) } + +func TestResolveWorkflowConfig(t *testing.T) { + tests := []struct { + name string + agentFlag string + repoCfg *config.RepoConfig + globalCfg *config.Config + want []string + }{ + { + name: "flag overrides all", + agentFlag: "codex,gemini", + repoCfg: &config.RepoConfig{ + Agent: "claude-code", + CI: config.RepoCIConfig{Agents: []string{"cursor"}}, + }, + globalCfg: &config.Config{ + DefaultAgent: "droid", + CI: config.CIConfig{Agents: []string{"kiro"}}, + }, + want: []string{"codex", "gemini"}, + }, + { + name: "repo ci agents override repo agent", + repoCfg: &config.RepoConfig{ + Agent: "claude-code", + CI: config.RepoCIConfig{Agents: []string{"gemini"}}, + }, + want: []string{"gemini"}, + }, + { + name: "repo agent overrides global ci agents", + repoCfg: &config.RepoConfig{ + Agent: "claude-code", + }, + globalCfg: &config.Config{ + CI: config.CIConfig{Agents: []string{"gemini"}}, + }, + want: []string{"claude-code"}, + }, + { + name: "global ci agents override global default agent", + globalCfg: &config.Config{ + DefaultAgent: "claude-code", + CI: config.CIConfig{Agents: []string{"gemini"}}, + }, + want: []string{"gemini"}, + }, + { + name: "global default agent beats built-in default", + globalCfg: &config.Config{ + DefaultAgent: "claude-code", + }, + want: []string{"claude-code"}, + }, + { + name: "falls back to built-in default", + want: []string{"codex"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := config.ResolveCIWorkflowAgents( + tt.agentFlag, tt.repoCfg, tt.globalCfg) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 3fd61211c..ee56d4c86 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -880,6 +880,64 @@ func resolve[T comparable](defaultVal T, candidates ...T) T { return defaultVal } +// resolveSlice returns the first non-empty slice from candidates, or defaultVal. +func resolveSlice[T any](defaultVal []T, candidates ...[]T) []T { + for _, v := range candidates { + if len(v) > 0 { + return v + } + } + return defaultVal +} + +// resolveBool returns the first non-nil boolean from candidates, or defaultVal. +func resolveBool(defaultVal bool, candidates ...*bool) bool { + for _, v := range candidates { + if v != nil { + return *v + } + } + return defaultVal +} + +func splitTrimmedCSV(value string) []string { + parts := strings.Split(value, ",") + out := make([]string, 0, len(parts)) + for _, part := range parts { + if trimmed := strings.TrimSpace(part); trimmed != "" { + out = append(out, trimmed) + } + } + return out +} + +func singleTrimmedValue(value string) []string { + if trimmed := strings.TrimSpace(value); trimmed != "" { + return []string{trimmed} + } + return nil +} + +func resolveNormalized( + defaultVal string, + normalize func(string) (string, error), + explicit string, + candidates ...string, +) (string, error) { + if strings.TrimSpace(explicit) != "" { + return normalize(explicit) + } + for _, candidate := range candidates { + if strings.TrimSpace(candidate) == "" { + continue + } + if normalized, err := normalize(candidate); err == nil && normalized != "" { + return normalized, nil + } + } + return defaultVal, nil +} + // ResolveAgent determines which agent to use based on config priority: // 1. Explicit agent parameter (if non-empty) // 2. Per-repo config @@ -897,6 +955,144 @@ func ResolveAgent(explicit string, repoPath string, globalCfg *Config) string { return resolve("codex", explicit, repoVal, globalVal) } +// ResolveCIAgents determines which agents to use for CI review execution. +// Priority: explicit CSV flag > repo [ci].agents > global [ci].agents > [""]. +func ResolveCIAgents( + explicit string, + repoCfg *RepoConfig, + globalCfg *Config, +) []string { + if explicit != "" { + return splitTrimmedCSV(explicit) + } + var repoAgents []string + if repoCfg != nil { + repoAgents = repoCfg.CI.Agents + } + var globalAgents []string + if globalCfg != nil { + globalAgents = globalCfg.CI.Agents + } + return resolveSlice([]string{""}, repoAgents, globalAgents) +} + +// ResolveCIReviewTypes determines which review types to use for CI review execution. +// Priority: explicit CSV flag > repo [ci].review_types > global [ci].review_types > ["security"]. +func ResolveCIReviewTypes( + explicit string, + repoCfg *RepoConfig, + globalCfg *Config, +) []string { + if explicit != "" { + return splitTrimmedCSV(explicit) + } + var repoTypes []string + if repoCfg != nil { + repoTypes = repoCfg.CI.ReviewTypes + } + var globalTypes []string + if globalCfg != nil { + globalTypes = globalCfg.CI.ReviewTypes + } + return resolveSlice([]string{ReviewTypeSecurity}, repoTypes, globalTypes) +} + +// ResolveCIReasoning determines the reasoning level for CI review execution. +// Priority: explicit > repo [ci].reasoning > "thorough". +func ResolveCIReasoning( + explicit string, + repoCfg *RepoConfig, + globalCfg *Config, +) (string, error) { + var repoVal string + if repoCfg != nil { + repoVal = repoCfg.CI.Reasoning + } + _ = globalCfg + return resolveNormalized("thorough", NormalizeReasoning, explicit, repoVal) +} + +// ResolveCIMinSeverity determines the synthesis severity filter for CI review execution. +// Priority: explicit > repo [ci].min_severity > global [ci].min_severity > "". +func ResolveCIMinSeverity( + explicit string, + repoCfg *RepoConfig, + globalCfg *Config, +) (string, error) { + var repoVal string + if repoCfg != nil { + repoVal = repoCfg.CI.MinSeverity + } + var globalVal string + if globalCfg != nil { + globalVal = globalCfg.CI.MinSeverity + } + return resolveNormalized("", NormalizeMinSeverity, explicit, repoVal, globalVal) +} + +// ResolveCISynthesisAgent determines the synthesis agent for CI review execution. +// Priority: explicit > global [ci].synthesis_agent > "". +func ResolveCISynthesisAgent( + explicit string, + repoCfg *RepoConfig, + globalCfg *Config, +) string { + var globalVal string + if globalCfg != nil { + globalVal = strings.TrimSpace(globalCfg.CI.SynthesisAgent) + } + _ = repoCfg + return resolve("", strings.TrimSpace(explicit), globalVal) +} + +// ResolveCIUpsertComments determines whether CI should update an existing PR comment. +// Priority: repo [ci].upsert_comments > global [ci].upsert_comments > false. +func ResolveCIUpsertComments( + repoCfg *RepoConfig, + globalCfg *Config, +) bool { + var repoVal *bool + if repoCfg != nil { + repoVal = repoCfg.CI.UpsertComments + } + var globalVal *bool + if globalCfg != nil { + globalVal = &globalCfg.CI.UpsertComments + } + return resolveBool(false, repoVal, globalVal) +} + +// ResolveCIWorkflowAgents determines which agents to encode into a generated CI workflow. +// Priority: explicit CSV flag > repo [ci].agents > repo agent > global [ci].agents > global default_agent > ["codex"]. +func ResolveCIWorkflowAgents( + explicit string, + repoCfg *RepoConfig, + globalCfg *Config, +) []string { + if explicit != "" { + return splitTrimmedCSV(explicit) + } + var repoCIAgents []string + var repoAgent []string + if repoCfg != nil { + repoCIAgents = repoCfg.CI.Agents + repoAgent = singleTrimmedValue(repoCfg.Agent) + } + var globalCIAgents []string + var globalAgent []string + if globalCfg != nil { + globalCIAgents = globalCfg.CI.Agents + globalAgent = singleTrimmedValue(globalCfg.DefaultAgent) + } + return resolveSlice( + []string{"codex"}, + repoCIAgents, + repoAgent, + globalCIAgents, + globalAgent, + ) +} + // clampPositive returns v if v > 0, otherwise 0. func clampPositive(v int) int { if v > 0 {