From 601befca45fc1035ef003c1804debf4f2e813b86 Mon Sep 17 00:00:00 2001 From: Phillip Cloud <417981+cpcloud@users.noreply.github.com> Date: Fri, 3 Apr 2026 09:06:14 -0400 Subject: [PATCH 1/8] fix: validate reasoning overrides and restore worktree fallback --- internal/config/config.go | 36 +++++++++++++++++++++ internal/config/config_test.go | 1 + internal/daemon/server.go | 10 +----- internal/daemon/server_actions_test.go | 43 +++++++++++++++++++------- internal/daemon/server_ops_test.go | 18 ++++++++--- 5 files changed, 83 insertions(+), 25 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 53f48ca1..cdd41d37 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,27 @@ 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 + } + + _, err = NormalizeReasoning(repoValue(repoCfg)) + 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..97f98e83 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -406,6 +406,7 @@ func TestResolveReasoning(t *testing.T) { {"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 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..77399fa7 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -747,11 +747,7 @@ func validatedWorktreePath(worktreePath, repoPath string) string { func resolveRerunModelProvider(job *storage.ReviewJob, cfg *config.Config) (string, string, error) { workflow := workflowForJob(job.JobType, job.ReviewType) resolutionPath := job.RepoPath - if job.WorktreePath != "" { - worktreePath := validatedWorktreePath(job.WorktreePath, job.RepoPath) - if worktreePath == "" { - return "", "", fmt.Errorf("rerun job worktree path is stale or invalid") - } + if worktreePath := validatedWorktreePath(job.WorktreePath, job.RepoPath); worktreePath != "" { resolutionPath = worktreePath } if err := config.ValidateRepoConfig(resolutionPath); err != nil { @@ -2493,10 +2489,6 @@ func (s *Server) handleFixJob(w http.ResponseWriter, r *http.Request) { cfg := s.configWatcher.Config() resolutionPath := parentJob.RepoPath worktreePath := validatedWorktreePath(parentJob.WorktreePath, parentJob.RepoPath) - if parentJob.WorktreePath != "" && worktreePath == "" { - writeError(w, http.StatusBadRequest, "parent job worktree path is stale or invalid") - return - } if worktreePath != "" { resolutionPath = worktreePath } diff --git a/internal/daemon/server_actions_test.go b/internal/daemon/server_actions_test.go index 2dc9b348..58d3847c 100644 --- a/internal/daemon/server_actions_test.go +++ b/internal/daemon/server_actions_test.go @@ -409,7 +409,7 @@ func TestHandleRerunJob(t *testing.T) { } }) - t.Run("rerun with invalid worktree path fails", func(t *testing.T) { + t.Run("rerun with invalid worktree path falls back to repo", func(t *testing.T) { repoDir := filepath.Join(tmpDir, "rerun-invalid-worktree") testutil.InitTestGitRepo(t, repoDir) @@ -441,15 +441,11 @@ func TestHandleRerunJob(t *testing.T) { w := httptest.NewRecorder() server.handleRerunJob(w, req) - testutil.AssertStatusCode(t, w, http.StatusBadRequest) - - var resp ErrorResponse - testutil.DecodeJSON(t, w, &resp) - assert.Contains(t, resp.Error, "rerun job worktree path is stale or invalid") + testutil.AssertStatusCode(t, w, http.StatusOK) updated, err := db.GetJobByID(job.ID) require.NoError(t, err) - assert.Equal(t, storage.JobStatusDone, updated.Status) + assert.Equal(t, storage.JobStatusQueued, updated.Status) }) t.Run("rerun nonexistent job fails", func(t *testing.T) { @@ -530,7 +526,7 @@ func TestResolveRerunModelProviderUsesWorktreeConfig(t *testing.T) { assert.Empty(t, provider) } -func TestResolveRerunModelProviderRejectsInvalidWorktreeConfig(t *testing.T) { +func TestResolveRerunModelProviderFallsBackOnInvalidWorktreeConfig(t *testing.T) { mainRepo := t.TempDir() stalePath := t.TempDir() @@ -549,12 +545,37 @@ func TestResolveRerunModelProviderRejectsInvalidWorktreeConfig(t *testing.T) { 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) + require.NoError(t, err) + assert.Equal(t, "main-model", model) assert.Empty(t, provider) } +func TestResolveRerunModelProviderPreservesRequestedOverridesOnInvalidWorktree(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.NoError(t, err) + assert.Equal(t, "requested-model", model) + assert.Equal(t, "anthropic", 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_ops_test.go b/internal/daemon/server_ops_test.go index e048a3c2..2c21689c 100644 --- a/internal/daemon/server_ops_test.go +++ b/internal/daemon/server_ops_test.go @@ -726,7 +726,7 @@ func TestHandleFixJobStaleValidation(t *testing.T) { require.Equal(t, worktreePath, stored.WorktreePath) }) - t.Run("fix job with invalid worktree path is rejected", func(t *testing.T) { + t.Run("fix job falls back from invalid worktree path", func(t *testing.T) { stalePath := filepath.Join(tmpDir, "worktrees", "stale-config") require.NoError(t, os.MkdirAll(stalePath, 0o755)) require.NoError(t, os.WriteFile( @@ -763,11 +763,19 @@ func TestHandleFixJobStaleValidation(t *testing.T) { ) w := httptest.NewRecorder() server.handleFixJob(w, req) - assertHandlerStatus(t, w, http.StatusBadRequest) + assertHandlerStatus(t, w, http.StatusCreated) - var resp ErrorResponse - testutil.DecodeJSON(t, w, &resp) - assert.Contains(t, resp.Error, "parent job worktree path is stale or invalid") + var fixJob storage.ReviewJob + testutil.DecodeJSON(t, w, &fixJob) + require.Equal(t, "standard", fixJob.Reasoning) + require.Empty(t, fixJob.Model) + require.Empty(t, fixJob.WorktreePath) + + stored, err := db.GetJobByID(fixJob.ID) + require.NoError(t, err) + require.Equal(t, "standard", stored.Reasoning) + require.Empty(t, stored.Model) + require.Empty(t, stored.WorktreePath) }) t.Run("custom prompt includes review context", func(t *testing.T) { From 70b4d36b0b47f08ca432459801abe87cc47077dc Mon Sep 17 00:00:00 2001 From: Phillip Cloud <417981+cpcloud@users.noreply.github.com> Date: Fri, 3 Apr 2026 09:27:23 -0400 Subject: [PATCH 2/8] daemon: align enqueue and rerun config paths --- internal/daemon/server.go | 20 +++++++--- internal/daemon/server_actions_test.go | 23 +++++++++++ internal/daemon/server_jobs_test.go | 54 ++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 6 deletions(-) diff --git a/internal/daemon/server.go b/internal/daemon/server.go index 77399fa7..7d8023fe 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -745,6 +745,11 @@ func validatedWorktreePath(worktreePath, repoPath string) string { } func resolveRerunModelProvider(job *storage.ReviewJob, cfg *config.Config) (string, string, error) { + provider := strings.TrimSpace(job.RequestedProvider) + if model := strings.TrimSpace(job.RequestedModel); model != "" { + return model, provider, nil + } + workflow := workflowForJob(job.JobType, job.ReviewType) resolutionPath := job.RepoPath if worktreePath := validatedWorktreePath(job.WorktreePath, job.RepoPath); worktreePath != "" { @@ -759,8 +764,7 @@ func resolveRerunModelProvider(job *storage.ReviewJob, cfg *config.Config) (stri 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 } @@ -878,15 +882,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()) @@ -897,12 +905,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 58d3847c..ac39d88a 100644 --- a/internal/daemon/server_actions_test.go +++ b/internal/daemon/server_actions_test.go @@ -576,6 +576,29 @@ func TestResolveRerunModelProviderPreservesRequestedOverridesOnInvalidWorktree(t assert.Equal(t, "anthropic", provider) } +func TestResolveRerunModelProviderPreservesRequestedOverridesOnInvalidConfig(t *testing.T) { + mainRepo := t.TempDir() + + require.NoError(t, os.WriteFile(filepath.Join(mainRepo, ".roborev.toml"), []byte("review_model = ["), 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) +} + // 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..ea0810b4 100644 --- a/internal/daemon/server_jobs_test.go +++ b/internal/daemon/server_jobs_test.go @@ -2769,6 +2769,60 @@ 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) + assert.Equal(t, "maximum", job.Reasoning) + assert.Equal(t, filepath.Clean(worktreeDir), job.WorktreePath) +} + func TestHandleListJobsSlashNormalization(t *testing.T) { server, db, tmpDir := newTestServer(t) From 9a752fcb8b523a5057de6fafb66f605fde970886 Mon Sep 17 00:00:00 2001 From: Phillip Cloud <417981+cpcloud@users.noreply.github.com> Date: Fri, 3 Apr 2026 11:48:18 -0400 Subject: [PATCH 3/8] fix: tighten stale worktree validation --- internal/config/config.go | 7 ++++++- internal/config/config_test.go | 1 + internal/daemon/server.go | 17 +++++++++++++---- internal/daemon/server_actions_test.go | 26 ++++++++++++++++---------- internal/daemon/server_jobs_test.go | 6 +++++- internal/daemon/server_ops_test.go | 18 +++++------------- 6 files changed, 46 insertions(+), 29 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index cdd41d37..777e07c1 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1556,7 +1556,12 @@ func validateRepoReasoningOverride( return nil } - _, err = NormalizeReasoning(repoValue(repoCfg)) + reasoning := strings.TrimSpace(repoValue(repoCfg)) + if reasoning == "" { + return nil + } + + _, err = NormalizeReasoning(reasoning) return err } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 97f98e83..33cc8b4b 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -405,6 +405,7 @@ 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}, diff --git a/internal/daemon/server.go b/internal/daemon/server.go index 7d8023fe..3b5eb5c0 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -745,16 +745,21 @@ func validatedWorktreePath(worktreePath, repoPath string) string { } func resolveRerunModelProvider(job *storage.ReviewJob, cfg *config.Config) (string, string, error) { + resolutionPath := job.RepoPath + if job.WorktreePath != "" { + worktreePath := validatedWorktreePath(job.WorktreePath, job.RepoPath) + if worktreePath == "" { + return "", "", fmt.Errorf("rerun job worktree path is stale or invalid") + } + resolutionPath = worktreePath + } + provider := strings.TrimSpace(job.RequestedProvider) if model := strings.TrimSpace(job.RequestedModel); model != "" { return model, provider, nil } workflow := workflowForJob(job.JobType, job.ReviewType) - resolutionPath := job.RepoPath - if worktreePath := validatedWorktreePath(job.WorktreePath, job.RepoPath); worktreePath != "" { - resolutionPath = worktreePath - } if err := config.ValidateRepoConfig(resolutionPath); err != nil { return "", "", fmt.Errorf("resolve workflow config: %w", err) } @@ -2497,6 +2502,10 @@ func (s *Server) handleFixJob(w http.ResponseWriter, r *http.Request) { cfg := s.configWatcher.Config() resolutionPath := parentJob.RepoPath worktreePath := validatedWorktreePath(parentJob.WorktreePath, parentJob.RepoPath) + if parentJob.WorktreePath != "" && worktreePath == "" { + writeError(w, http.StatusBadRequest, "parent job worktree path is stale or invalid") + return + } if worktreePath != "" { resolutionPath = worktreePath } diff --git a/internal/daemon/server_actions_test.go b/internal/daemon/server_actions_test.go index ac39d88a..738887db 100644 --- a/internal/daemon/server_actions_test.go +++ b/internal/daemon/server_actions_test.go @@ -409,7 +409,7 @@ func TestHandleRerunJob(t *testing.T) { } }) - t.Run("rerun with invalid worktree path falls back to repo", func(t *testing.T) { + t.Run("rerun with invalid worktree path fails", func(t *testing.T) { repoDir := filepath.Join(tmpDir, "rerun-invalid-worktree") testutil.InitTestGitRepo(t, repoDir) @@ -441,11 +441,15 @@ func TestHandleRerunJob(t *testing.T) { w := httptest.NewRecorder() server.handleRerunJob(w, req) - testutil.AssertStatusCode(t, w, http.StatusOK) + testutil.AssertStatusCode(t, w, http.StatusBadRequest) + + var resp ErrorResponse + testutil.DecodeJSON(t, w, &resp) + assert.Contains(t, resp.Error, "rerun job worktree path is stale or invalid") updated, err := db.GetJobByID(job.ID) require.NoError(t, err) - assert.Equal(t, storage.JobStatusQueued, updated.Status) + assert.Equal(t, storage.JobStatusDone, updated.Status) }) t.Run("rerun nonexistent job fails", func(t *testing.T) { @@ -526,7 +530,7 @@ func TestResolveRerunModelProviderUsesWorktreeConfig(t *testing.T) { assert.Empty(t, provider) } -func TestResolveRerunModelProviderFallsBackOnInvalidWorktreeConfig(t *testing.T) { +func TestResolveRerunModelProviderRejectsInvalidWorktreeConfig(t *testing.T) { mainRepo := t.TempDir() stalePath := t.TempDir() @@ -545,12 +549,13 @@ func TestResolveRerunModelProviderFallsBackOnInvalidWorktreeConfig(t *testing.T) model, provider, err := resolveRerunModelProvider( job, config.DefaultConfig(), ) - require.NoError(t, err) - assert.Equal(t, "main-model", model) + 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 TestResolveRerunModelProviderPreservesRequestedOverridesOnInvalidWorktree(t *testing.T) { +func TestResolveRerunModelProviderRejectsInvalidWorktreeWithRequestedOverrides(t *testing.T) { mainRepo := t.TempDir() stalePath := t.TempDir() @@ -571,9 +576,10 @@ func TestResolveRerunModelProviderPreservesRequestedOverridesOnInvalidWorktree(t model, provider, err := resolveRerunModelProvider( job, config.DefaultConfig(), ) - require.NoError(t, err) - assert.Equal(t, "requested-model", model) - assert.Equal(t, "anthropic", provider) + 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 TestResolveRerunModelProviderPreservesRequestedOverridesOnInvalidConfig(t *testing.T) { diff --git a/internal/daemon/server_jobs_test.go b/internal/daemon/server_jobs_test.go index ea0810b4..425f648f 100644 --- a/internal/daemon/server_jobs_test.go +++ b/internal/daemon/server_jobs_test.go @@ -2819,8 +2819,12 @@ func TestHandleEnqueueUsesWorktreeConfigWhenPresent(t *testing.T) { 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(worktreeDir), job.WorktreePath) + assert.Equal(t, filepath.Clean(resolvedWorktreeDir), job.WorktreePath) } func TestHandleListJobsSlashNormalization(t *testing.T) { diff --git a/internal/daemon/server_ops_test.go b/internal/daemon/server_ops_test.go index 2c21689c..e048a3c2 100644 --- a/internal/daemon/server_ops_test.go +++ b/internal/daemon/server_ops_test.go @@ -726,7 +726,7 @@ func TestHandleFixJobStaleValidation(t *testing.T) { require.Equal(t, worktreePath, stored.WorktreePath) }) - t.Run("fix job falls back from invalid worktree path", func(t *testing.T) { + t.Run("fix job with invalid worktree path is rejected", func(t *testing.T) { stalePath := filepath.Join(tmpDir, "worktrees", "stale-config") require.NoError(t, os.MkdirAll(stalePath, 0o755)) require.NoError(t, os.WriteFile( @@ -763,19 +763,11 @@ func TestHandleFixJobStaleValidation(t *testing.T) { ) w := httptest.NewRecorder() server.handleFixJob(w, req) - assertHandlerStatus(t, w, http.StatusCreated) - - var fixJob storage.ReviewJob - testutil.DecodeJSON(t, w, &fixJob) - require.Equal(t, "standard", fixJob.Reasoning) - require.Empty(t, fixJob.Model) - require.Empty(t, fixJob.WorktreePath) + assertHandlerStatus(t, w, http.StatusBadRequest) - stored, err := db.GetJobByID(fixJob.ID) - require.NoError(t, err) - require.Equal(t, "standard", stored.Reasoning) - require.Empty(t, stored.Model) - require.Empty(t, stored.WorktreePath) + var resp ErrorResponse + testutil.DecodeJSON(t, w, &resp) + assert.Contains(t, resp.Error, "parent job worktree path is stale or invalid") }) t.Run("custom prompt includes review context", func(t *testing.T) { From 8ac85c56dce1e4d7237b4c986a58a15b6c1a6beb Mon Sep 17 00:00:00 2001 From: Phillip Cloud <417981+cpcloud@users.noreply.github.com> Date: Fri, 3 Apr 2026 12:18:02 -0400 Subject: [PATCH 4/8] rerun: validate requested-model agents --- internal/daemon/server.go | 30 ++++++++++++++++++++------ internal/daemon/server_actions_test.go | 22 +++++++++++++++++++ 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/internal/daemon/server.go b/internal/daemon/server.go index 3b5eb5c0..c7c6cd23 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -754,25 +754,41 @@ func resolveRerunModelProvider(job *storage.ReviewJob, cfg *config.Config) (stri resolutionPath = worktreePath } + 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) + } + if err := validateRerunAgent(job.Agent, cfg); err != nil { + return "", "", err + } + provider := strings.TrimSpace(job.RequestedProvider) if model := strings.TrimSpace(job.RequestedModel); model != "" { return model, provider, nil } - workflow := workflowForJob(job.JobType, job.ReviewType) if err := config.ValidateRepoConfig(resolutionPath); err != nil { return "", "", fmt.Errorf("resolve workflow config: %w", err) } - 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, "") 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") diff --git a/internal/daemon/server_actions_test.go b/internal/daemon/server_actions_test.go index 738887db..373e1c4a 100644 --- a/internal/daemon/server_actions_test.go +++ b/internal/daemon/server_actions_test.go @@ -605,6 +605,28 @@ func TestResolveRerunModelProviderPreservesRequestedOverridesOnInvalidConfig(t * assert.Equal(t, "anthropic", 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) { From 6c90c2b82a0410293c420dae895375ae6224cbb5 Mon Sep 17 00:00:00 2001 From: Phillip Cloud <417981+cpcloud@users.noreply.github.com> Date: Sat, 4 Apr 2026 08:28:58 -0400 Subject: [PATCH 5/8] rerun: short-circuit requested model overrides --- internal/daemon/server.go | 17 +++++++++-------- internal/daemon/server_actions_test.go | 4 ++-- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/internal/daemon/server.go b/internal/daemon/server.go index c7c6cd23..4f144076 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -745,6 +745,10 @@ func validatedWorktreePath(worktreePath, repoPath string) string { } func resolveRerunModelProvider(job *storage.ReviewJob, cfg *config.Config) (string, string, error) { + if err := validateRerunAgent(job.Agent, cfg); err != nil { + return "", "", err + } + resolutionPath := job.RepoPath if job.WorktreePath != "" { worktreePath := validatedWorktreePath(job.WorktreePath, job.RepoPath) @@ -754,6 +758,11 @@ func resolveRerunModelProvider(job *storage.ReviewJob, cfg *config.Config) (stri resolutionPath = worktreePath } + 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, @@ -761,14 +770,6 @@ func resolveRerunModelProvider(job *storage.ReviewJob, cfg *config.Config) (stri if err != nil { return "", "", fmt.Errorf("resolve workflow config: %w", err) } - if err := validateRerunAgent(job.Agent, cfg); err != nil { - return "", "", err - } - - provider := strings.TrimSpace(job.RequestedProvider) - if model := strings.TrimSpace(job.RequestedModel); model != "" { - return model, provider, nil - } if err := config.ValidateRepoConfig(resolutionPath); err != nil { return "", "", fmt.Errorf("resolve workflow config: %w", err) diff --git a/internal/daemon/server_actions_test.go b/internal/daemon/server_actions_test.go index 373e1c4a..df190a66 100644 --- a/internal/daemon/server_actions_test.go +++ b/internal/daemon/server_actions_test.go @@ -582,10 +582,10 @@ func TestResolveRerunModelProviderRejectsInvalidWorktreeWithRequestedOverrides(t assert.Empty(t, provider) } -func TestResolveRerunModelProviderPreservesRequestedOverridesOnInvalidConfig(t *testing.T) { +func TestResolveRerunModelProviderPreservesRequestedOverridesOnParseableInvalidConfig(t *testing.T) { mainRepo := t.TempDir() - require.NoError(t, os.WriteFile(filepath.Join(mainRepo, ".roborev.toml"), []byte("review_model = ["), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(mainRepo, ".roborev.toml"), []byte("review_reasoning = \"bogus\"\n"), 0o644)) job := &storage.ReviewJob{ Agent: "test", From 54ae56c7dbdec04734b0ab01ec1e0021995e9218 Mon Sep 17 00:00:00 2001 From: Phillip Cloud <417981+cpcloud@users.noreply.github.com> Date: Sat, 4 Apr 2026 08:52:19 -0400 Subject: [PATCH 6/8] daemon: stabilize rerun agent test --- internal/daemon/server_actions_test.go | 46 ++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/internal/daemon/server_actions_test.go b/internal/daemon/server_actions_test.go index df190a66..e4d25db9 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() From 6c29111b5e5c456f56ab50ef31d3cafa99d2e17b Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 5 Apr 2026 08:11:20 -0500 Subject: [PATCH 7/8] rerun: validate repo config before model short-circuit Move ValidateRepoConfig before the RequestedModel early-return so malformed .roborev.toml is caught on rerun regardless of whether an explicit model override is present, matching the enqueue path behavior. Co-Authored-By: Claude Opus 4.6 (1M context) --- internal/daemon/server.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/daemon/server.go b/internal/daemon/server.go index 4f144076..f476d113 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -758,6 +758,10 @@ 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 @@ -770,10 +774,6 @@ func resolveRerunModelProvider(job *storage.ReviewJob, cfg *config.Config) (stri if err != nil { return "", "", fmt.Errorf("resolve workflow config: %w", err) } - - if err := config.ValidateRepoConfig(resolutionPath); err != nil { - return "", "", fmt.Errorf("resolve workflow config: %w", err) - } model := resolution.ModelForSelectedAgent(job.Agent, "") return model, provider, nil } From 735623e318ff5e40fd58cb3736660360addf0f1c Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 5 Apr 2026 08:26:54 -0500 Subject: [PATCH 8/8] rerun: add regression test for malformed config with model override Verify that resolveRerunModelProvider rejects a syntactically malformed .roborev.toml even when RequestedModel is set, preventing the short-circuit from silently skipping config validation. Co-Authored-By: Claude Opus 4.6 (1M context) --- internal/daemon/server_actions_test.go | 28 ++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/internal/daemon/server_actions_test.go b/internal/daemon/server_actions_test.go index e4d25db9..39c60034 100644 --- a/internal/daemon/server_actions_test.go +++ b/internal/daemon/server_actions_test.go @@ -637,6 +637,34 @@ func TestResolveRerunModelProviderPreservesRequestedOverridesOnParseableInvalidC 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()