diff --git a/internal/config/config.go b/internal/config/config.go index 53f48ca1..777e07c1 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1462,6 +1462,11 @@ func SeverityInstruction(minSeverity string) string { // Priority: explicit > per-repo config > global config > default (thorough) func ResolveReviewReasoning(explicit string, repoPath string, globalCfg *Config) (string, error) { if strings.TrimSpace(explicit) != "" { + if err := validateRepoReasoningOverride(repoPath, func(cfg *RepoConfig) string { + return cfg.ReviewReasoning + }); err != nil { + return "", err + } return NormalizeReasoning(explicit) } @@ -1484,6 +1489,11 @@ func ResolveReviewReasoning(explicit string, repoPath string, globalCfg *Config) // Priority: explicit > per-repo config > global config > default (standard) func ResolveRefineReasoning(explicit string, repoPath string, globalCfg *Config) (string, error) { if strings.TrimSpace(explicit) != "" { + if err := validateRepoReasoningOverride(repoPath, func(cfg *RepoConfig) string { + return cfg.RefineReasoning + }); err != nil { + return "", err + } return NormalizeReasoning(explicit) } @@ -1506,6 +1516,11 @@ func ResolveRefineReasoning(explicit string, repoPath string, globalCfg *Config) // Priority: explicit > per-repo config > global config > default (standard) func ResolveFixReasoning(explicit string, repoPath string, globalCfg *Config) (string, error) { if strings.TrimSpace(explicit) != "" { + if err := validateRepoReasoningOverride(repoPath, func(cfg *RepoConfig) string { + return cfg.FixReasoning + }); err != nil { + return "", err + } return NormalizeReasoning(explicit) } @@ -1524,6 +1539,32 @@ func ResolveFixReasoning(explicit string, repoPath string, globalCfg *Config) (s return "standard", nil // Default for fix: balanced analysis } +func validateRepoReasoningOverride( + repoPath string, + repoValue func(*RepoConfig) string, +) error { + if strings.TrimSpace(repoPath) == "" { + return nil + } + + repoCfg, err := LoadRepoConfig(repoPath) + // Entry points that must fail fast on malformed .roborev.toml call + // ValidateRepoConfig separately. Here we only want to catch a parseable + // but invalid workflow reasoning override before an explicit CLI value + // silently masks it. + if err != nil || repoCfg == nil { + return nil + } + + reasoning := strings.TrimSpace(repoValue(repoCfg)) + if reasoning == "" { + return nil + } + + _, err = NormalizeReasoning(reasoning) + return err +} + // ResolveFixMinSeverity determines minimum severity for fix. // Priority: explicit > per-repo config > "" (no filter) func ResolveFixMinSeverity( diff --git a/internal/config/config_test.go b/internal/config/config_test.go index a07b0f60..33cc8b4b 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -405,7 +405,9 @@ func TestResolveReasoning(t *testing.T) { {"repo config overrides global config", "", fmt.Sprintf(`%s = "%s"`, configKey, repoVal), fmt.Sprintf(`%s = "%s"`, configKey, defaultVal), repoVal, false}, {"explicit overrides repo config", "fast", fmt.Sprintf(`%s = "%s"`, configKey, repoVal), fmt.Sprintf(`%s = "%s"`, configKey, defaultVal), "fast", false}, {"explicit normalization", "FAST", "", "", "fast", false}, + {"explicit with valid repo config but no reasoning override", "fast", "# valid config", "", "fast", false}, {"explicit bypasses malformed repo config", "fast", fmt.Sprintf(`%s = [`, configKey), "", "fast", false}, + {"explicit does not bypass invalid repo config", "fast", fmt.Sprintf(`%s = "invalid"`, configKey), "", "", true}, {"invalid explicit", "unknown", "", "", "", true}, {"invalid repo config", "", fmt.Sprintf(`%s = "invalid"`, configKey), "", "", true}, {"malformed repo config does not fall back to global", "", fmt.Sprintf(`%s = [`, configKey), fmt.Sprintf(`%s = "%s"`, configKey, repoVal), "", true}, diff --git a/internal/daemon/server.go b/internal/daemon/server.go index 23d62051..f476d113 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -745,7 +745,10 @@ func validatedWorktreePath(worktreePath, repoPath string) string { } func resolveRerunModelProvider(job *storage.ReviewJob, cfg *config.Config) (string, string, error) { - workflow := workflowForJob(job.JobType, job.ReviewType) + if err := validateRerunAgent(job.Agent, cfg); err != nil { + return "", "", err + } + resolutionPath := job.RepoPath if job.WorktreePath != "" { worktreePath := validatedWorktreePath(job.WorktreePath, job.RepoPath) @@ -754,20 +757,39 @@ func resolveRerunModelProvider(job *storage.ReviewJob, cfg *config.Config) (stri } resolutionPath = worktreePath } + if err := config.ValidateRepoConfig(resolutionPath); err != nil { return "", "", fmt.Errorf("resolve workflow config: %w", err) } + + provider := strings.TrimSpace(job.RequestedProvider) + if model := strings.TrimSpace(job.RequestedModel); model != "" { + return model, provider, nil + } + + workflow := workflowForJob(job.JobType, job.ReviewType) resolution, err := agent.ResolveWorkflowConfig( "", resolutionPath, cfg, workflow, job.Reasoning, ) if err != nil { return "", "", fmt.Errorf("resolve workflow config: %w", err) } - model := resolution.ModelForSelectedAgent(job.Agent, job.RequestedModel) - provider := strings.TrimSpace(job.RequestedProvider) + model := resolution.ModelForSelectedAgent(job.Agent, "") return model, provider, nil } +func validateRerunAgent(agentName string, cfg *config.Config) error { + _, err := agent.GetAvailableWithConfig(agentName, cfg) + if err != nil { + var unknownErr *agent.UnknownAgentError + if errors.As(err, &unknownErr) { + return fmt.Errorf("invalid agent: %w", err) + } + return fmt.Errorf("no agent available: %w", err) + } + return nil +} + func (s *Server) handleEnqueue(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodPost { writeError(w, http.StatusMethodNotAllowed, "method not allowed") @@ -882,15 +904,19 @@ func (s *Server) handleEnqueue(w http.ResponseWriter, r *http.Request) { workflow := workflowForJob(req.JobType, req.ReviewType) cfg := s.configWatcher.Config() + resolutionPath := repoRoot + if worktreePath != "" { + resolutionPath = worktreePath + } // Resolve reasoning level for the determined workflow. // Compact jobs use fix reasoning (default "standard"), not review // reasoning (default "thorough"). var reasoning string if workflow == "fix" { - reasoning, err = config.ResolveFixReasoning(req.Reasoning, repoRoot, cfg) + reasoning, err = config.ResolveFixReasoning(req.Reasoning, resolutionPath, cfg) } else { - reasoning, err = config.ResolveReviewReasoning(req.Reasoning, repoRoot, cfg) + reasoning, err = config.ResolveReviewReasoning(req.Reasoning, resolutionPath, cfg) } if err != nil { writeError(w, http.StatusBadRequest, err.Error()) @@ -901,12 +927,12 @@ func (s *Server) handleEnqueue(w http.ResponseWriter, r *http.Request) { requestedProvider := strings.TrimSpace(req.Provider) // Resolve agent for workflow at this reasoning level - if err := config.ValidateRepoConfig(repoRoot); err != nil { + if err := config.ValidateRepoConfig(resolutionPath); err != nil { writeError(w, http.StatusBadRequest, fmt.Sprintf("resolve workflow config: %v", err)) return } resolution, err := agent.ResolveWorkflowConfig( - req.Agent, repoRoot, cfg, workflow, reasoning, + req.Agent, resolutionPath, cfg, workflow, reasoning, ) if err != nil { writeError(w, http.StatusBadRequest, fmt.Sprintf("resolve workflow config: %v", err)) diff --git a/internal/daemon/server_actions_test.go b/internal/daemon/server_actions_test.go index 2dc9b348..39c60034 100644 --- a/internal/daemon/server_actions_test.go +++ b/internal/daemon/server_actions_test.go @@ -1,11 +1,8 @@ package daemon import ( - "github.com/roborev-dev/roborev/internal/config" - "github.com/roborev-dev/roborev/internal/storage" - "github.com/roborev-dev/roborev/internal/testutil" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" + "context" + "io" "net/http" "net/http/httptest" "os" @@ -14,8 +11,38 @@ import ( "strings" "testing" "time" + + "github.com/roborev-dev/roborev/internal/agent" + "github.com/roborev-dev/roborev/internal/config" + "github.com/roborev-dev/roborev/internal/storage" + "github.com/roborev-dev/roborev/internal/testutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +type commandTestAgent struct { + name string + command string +} + +func (a *commandTestAgent) Name() string { return a.name } + +func (a *commandTestAgent) Review(ctx context.Context, repoPath, commitSHA, prompt string, output io.Writer) (string, error) { + return "No issues found.", nil +} + +func (a *commandTestAgent) WithReasoning(level agent.ReasoningLevel) agent.Agent { + return a +} + +func (a *commandTestAgent) WithAgentic(agentic bool) agent.Agent { return a } + +func (a *commandTestAgent) WithModel(model string) agent.Agent { return a } + +func (a *commandTestAgent) CommandLine() string { return a.command } + +func (a *commandTestAgent) CommandName() string { return a.command } + func TestHandleStatus(t *testing.T) { server, _, _ := newTestServer(t) @@ -361,6 +388,11 @@ func TestHandleRerunJob(t *testing.T) { t.Run("rerun reevaluates implicit effective model", func(t *testing.T) { isolatedDB, isolatedDir := testutil.OpenTestDBWithDir(t) server := NewServer(isolatedDB, config.DefaultConfig(), "") + agentName := "rerun-implicit-model" + agent.Register(&commandTestAgent{name: agentName, command: "go"}) + t.Cleanup(func() { + agent.Unregister(agentName) + }) repo, err := isolatedDB.GetOrCreateRepo(isolatedDir) require.NoError(t, err) @@ -370,7 +402,7 @@ func TestHandleRerunJob(t *testing.T) { RepoID: repo.ID, CommitID: commit.ID, GitRef: "rerun-implicit-model", - Agent: "opencode", + Agent: agentName, Model: "minimax-m2.5-free", }) require.NoError(t, err) @@ -379,7 +411,7 @@ func TestHandleRerunJob(t *testing.T) { require.NoError(t, err) require.NotNil(t, claimed) require.Equal(t, job.ID, claimed.ID) - require.NoError(t, isolatedDB.CompleteJob(job.ID, "opencode", "prompt", "output")) + require.NoError(t, isolatedDB.CompleteJob(job.ID, agentName, "prompt", "output")) req := testutil.MakeJSONRequest(t, http.MethodPost, "/api/job/rerun", RerunJobRequest{JobID: job.ID}) w := httptest.NewRecorder() @@ -555,6 +587,106 @@ func TestResolveRerunModelProviderRejectsInvalidWorktreeConfig(t *testing.T) { assert.Empty(t, provider) } +func TestResolveRerunModelProviderRejectsInvalidWorktreeWithRequestedOverrides(t *testing.T) { + mainRepo := t.TempDir() + stalePath := t.TempDir() + + require.NoError(t, os.WriteFile(filepath.Join(mainRepo, ".roborev.toml"), []byte("review_model = \"main-model\"\n"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(stalePath, ".roborev.toml"), []byte("review_model = \"stale-model\"\n"), 0o644)) + + job := &storage.ReviewJob{ + Agent: "test", + JobType: storage.JobTypeReview, + ReviewType: config.ReviewTypeDefault, + Reasoning: "thorough", + RepoPath: mainRepo, + WorktreePath: stalePath, + RequestedModel: "requested-model", + RequestedProvider: "anthropic", + } + + model, provider, err := resolveRerunModelProvider( + job, config.DefaultConfig(), + ) + require.Error(t, err) + require.ErrorContains(t, err, "rerun job worktree path is stale or invalid") + assert.Empty(t, model) + assert.Empty(t, provider) +} + +func TestResolveRerunModelProviderPreservesRequestedOverridesOnParseableInvalidConfig(t *testing.T) { + mainRepo := t.TempDir() + + require.NoError(t, os.WriteFile(filepath.Join(mainRepo, ".roborev.toml"), []byte("review_reasoning = \"bogus\"\n"), 0o644)) + + job := &storage.ReviewJob{ + Agent: "test", + JobType: storage.JobTypeReview, + ReviewType: config.ReviewTypeDefault, + Reasoning: "thorough", + RepoPath: mainRepo, + RequestedModel: "requested-model", + RequestedProvider: "anthropic", + } + + model, provider, err := resolveRerunModelProvider( + job, config.DefaultConfig(), + ) + require.NoError(t, err) + assert.Equal(t, "requested-model", model) + assert.Equal(t, "anthropic", provider) +} + +func TestResolveRerunModelProviderRejectsMalformedConfigWithRequestedOverrides(t *testing.T) { + mainRepo := t.TempDir() + + require.NoError(t, os.WriteFile( + filepath.Join(mainRepo, ".roborev.toml"), + []byte("this is not valid toml [[["), + 0o644, + )) + + job := &storage.ReviewJob{ + Agent: "test", + JobType: storage.JobTypeReview, + ReviewType: config.ReviewTypeDefault, + Reasoning: "thorough", + RepoPath: mainRepo, + RequestedModel: "requested-model", + RequestedProvider: "anthropic", + } + + model, provider, err := resolveRerunModelProvider( + job, config.DefaultConfig(), + ) + require.Error(t, err) + require.ErrorContains(t, err, "resolve workflow config") + assert.Empty(t, model) + assert.Empty(t, provider) +} + +func TestResolveRerunModelProviderRejectsInvalidAgentWithRequestedOverrides(t *testing.T) { + mainRepo := t.TempDir() + + job := &storage.ReviewJob{ + Agent: "missing-agent", + JobType: storage.JobTypeReview, + ReviewType: config.ReviewTypeDefault, + Reasoning: "thorough", + RepoPath: mainRepo, + RequestedModel: "requested-model", + RequestedProvider: "anthropic", + } + + model, provider, err := resolveRerunModelProvider( + job, config.DefaultConfig(), + ) + require.Error(t, err) + require.ErrorContains(t, err, `invalid agent: unknown agent "missing-agent"`) + assert.Empty(t, model) + assert.Empty(t, provider) +} + // TestHandleAddCommentToJobStates tests that comments can be added to jobs // in any state: queued, running, done, failed, and canceled. func TestHandleAddCommentToJobStates(t *testing.T) { diff --git a/internal/daemon/server_jobs_test.go b/internal/daemon/server_jobs_test.go index 927a599d..425f648f 100644 --- a/internal/daemon/server_jobs_test.go +++ b/internal/daemon/server_jobs_test.go @@ -2769,6 +2769,64 @@ func TestHandleEnqueueRejectsMalformedRepoConfigWithExplicitReasoning(t *testing assert.Contains(t, w.Body.String(), "resolve workflow config:") } +func TestHandleEnqueueUsesWorktreeConfigWhenPresent(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("skipping worktree test on Windows due to path differences") + } + + _ = testenv.SetDataDir(t) + + tmpDir := t.TempDir() + mainRepo := filepath.Join(tmpDir, "main-repo") + testutil.InitTestGitRepo(t, mainRepo) + + worktreeDir := filepath.Join(tmpDir, "worktree") + wtCmd := exec.Command( + "git", "-C", mainRepo, "worktree", "add", "-b", "wt-branch", + worktreeDir, + ) + out, err := wtCmd.CombinedOutput() + require.NoError(t, err, "git worktree add failed: %s", out) + + err = os.WriteFile( + filepath.Join(mainRepo, ".roborev.toml"), + []byte("review_model = ["), + 0o644, + ) + require.NoError(t, err) + err = os.WriteFile( + filepath.Join(worktreeDir, ".roborev.toml"), + []byte(`review_reasoning = "maximum"`), + 0o644, + ) + require.NoError(t, err) + + db, _ := testutil.OpenTestDBWithDir(t) + server := NewServer(db, config.DefaultConfig(), "") + t.Cleanup(func() { + require.NoError(t, server.Close()) + }) + + req := testutil.MakeJSONRequest(t, http.MethodPost, "/api/enqueue", EnqueueRequest{ + RepoPath: worktreeDir, + GitRef: "HEAD", + Agent: "test", + }) + w := httptest.NewRecorder() + server.handleEnqueue(w, req) + + require.Equal(t, http.StatusCreated, w.Code, w.Body.String()) + + var job storage.ReviewJob + testutil.DecodeJSON(t, w, &job) + resolvedWorktreeDir, err := filepath.EvalSymlinks(worktreeDir) + if err != nil { + resolvedWorktreeDir = worktreeDir + } + assert.Equal(t, "maximum", job.Reasoning) + assert.Equal(t, filepath.Clean(resolvedWorktreeDir), job.WorktreePath) +} + func TestHandleListJobsSlashNormalization(t *testing.T) { server, db, tmpDir := newTestServer(t)