Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions internal/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand All @@ -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)
}

Expand All @@ -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)
}

Expand All @@ -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(
Expand Down
2 changes: 2 additions & 0 deletions internal/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down
40 changes: 33 additions & 7 deletions internal/daemon/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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")
Expand Down Expand Up @@ -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())
Expand All @@ -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))
Expand Down
146 changes: 139 additions & 7 deletions internal/daemon/server_actions_test.go
Original file line number Diff line number Diff line change
@@ -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"
Expand All @@ -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)

Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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()
Expand Down Expand Up @@ -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) {
Expand Down
Loading
Loading