Skip to content

Commit ff9af2e

Browse files
authored
feat: min-severity cascading and review-level filter (#635)
## Summary - Add `min_severity` config cascade (CLI flag → repo `.roborev.toml` → global `config.toml`) for review, refine, and fix workflows - Persist `min_severity` on `review_jobs` (SQLite + PostgreSQL v11), thread through enqueue/claim/sync paths - Inject severity instructions into review and fix prompts via `config.SeverityInstruction` - Parse `SEVERITY_THRESHOLD_MET` marker as a pass verdict - CI path: prompt uses `review_min_severity`, synthesis uses `[ci].min_severity`; single-result CI runs route through synthesis when CI threshold is set - `formatSingleResult` uses `storage.ParseVerdict` to detect pass output (not loose `strings.Contains`) - Fix `GetJobsWithReviewsByIDs` hydration bug where `min_severity` was scanned then overwritten by zero-value field Co-authored-by: Phillip Cloud <cpcloud@users.noreply.github.com>
1 parent ef9d858 commit ff9af2e

31 files changed

Lines changed: 1096 additions & 222 deletions

cmd/roborev/ci.go

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -189,31 +189,40 @@ func runCIReview(ctx context.Context, opts ciReviewOpts) error {
189189
return err
190190
}
191191

192-
// Resolve min severity
193-
minSev, err := config.ResolveCIMinSeverity(
192+
// Resolve min severity for synthesis output filtering
193+
ciMinSev, err := config.ResolveCIMinSeverity(
194194
opts.minSeverity, repoCfg, globalCfg)
195195
if err != nil {
196196
return err
197197
}
198198

199+
// Resolve review-level min severity for prompt injection
200+
reviewMinSev, err := config.ResolveReviewMinSeverity(
201+
"", root, globalCfg)
202+
if err != nil {
203+
return err
204+
}
205+
199206
// Resolve synthesis agent
200207
synthAgent := config.ResolveCISynthesisAgent(
201208
opts.synthesisAgent, repoCfg, globalCfg)
202209

203210
log.Printf(
204211
"ci review: ref=%s agents=%v types=%v "+
205-
"reasoning=%s min_severity=%s",
212+
"reasoning=%s ci_min_severity=%s review_min_severity=%s",
206213
gitRef, agents, reviewTypes,
207-
reasoningLevel, minSev)
214+
reasoningLevel, ciMinSev, reviewMinSev)
208215

209-
// Run batch
216+
// Run batch with review-level severity for prompt injection.
217+
// CI-level severity is applied separately during synthesis.
210218
batchCfg := review.BatchConfig{
211219
RepoPath: root,
212220
GitRef: gitRef,
213221
Agents: agents,
214222
ReviewTypes: reviewTypes,
215223
Reasoning: reasoningLevel,
216224
GlobalConfig: globalCfg,
225+
MinSeverity: reviewMinSev,
217226
}
218227

219228
results := review.RunBatch(ctx, batchCfg)
@@ -225,7 +234,7 @@ func runCIReview(ctx context.Context, opts ciReviewOpts) error {
225234
comment, synthErr := review.Synthesize(
226235
ctx, results, review.SynthesizeOpts{
227236
Agent: synthAgent,
228-
MinSeverity: minSev,
237+
MinSeverity: ciMinSev,
229238
RepoPath: root,
230239
GitRef: gitRef,
231240
HeadSHA: headSHA,

cmd/roborev/fix.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -799,8 +799,9 @@ func fixSingleJob(cmd *cobra.Command, repoRoot string, jobID int64, opts fixOpti
799799
// task/analyze jobs have free-form output without severity labels)
800800
var minSev string
801801
if !job.IsTaskJob() {
802+
fixCfg, _ := config.LoadGlobal()
802803
minSev, err = config.ResolveFixMinSeverity(
803-
opts.minSeverity, repoRoot,
804+
opts.minSeverity, repoRoot, fixCfg,
804805
)
805806
if err != nil {
806807
return fmt.Errorf("resolve min-severity: %w", err)
@@ -1011,11 +1012,14 @@ func runFixBatch(cmd *cobra.Command, jobIDs []int64, branch string, allBranches,
10111012
return err
10121013
}
10131014

1015+
// Load global config for severity + prompt-size resolution.
1016+
cfg, _ := config.LoadGlobal()
1017+
10141018
// Resolve minimum severity filter. Suppress if any entry is a
10151019
// task job — task/analyze output has no severity labels, so the
10161020
// instruction would confuse the agent for those entries.
10171021
minSev, err := config.ResolveFixMinSeverity(
1018-
opts.minSeverity, roots.worktreeRoot,
1022+
opts.minSeverity, roots.worktreeRoot, cfg,
10191023
)
10201024
if err != nil {
10211025
return fmt.Errorf("resolve min-severity: %w", err)
@@ -1031,7 +1035,6 @@ func runFixBatch(cmd *cobra.Command, jobIDs []int64, branch string, allBranches,
10311035

10321036
// Split into batches by prompt size (after severity resolution
10331037
// so the severity instruction overhead is accounted for)
1034-
cfg, _ := config.LoadGlobal()
10351038
maxSize := config.ResolveMaxPromptSize(roots.worktreeRoot, cfg)
10361039
batches := splitIntoBatches(entries, maxSize, minSev)
10371040

cmd/roborev/refine.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -394,7 +394,7 @@ func runRefine(ctx RunContext, opts refineOptions) error {
394394

395395
// Resolve minimum severity filter
396396
minSev, err := config.ResolveRefineMinSeverity(
397-
opts.minSeverity, repoPath,
397+
opts.minSeverity, repoPath, cfg,
398398
)
399399
if err != nil {
400400
return fmt.Errorf("resolve min-severity: %w", err)
@@ -616,10 +616,10 @@ func runRefine(ctx RunContext, opts refineOptions) error {
616616

617617
// When severity filtering is active and the agent
618618
// signals all findings are below threshold, treat as
619-
// resolved rather than a fix failure.
620-
if minSev != "" && strings.Contains(
621-
output, config.SeverityThresholdMarker,
622-
) {
619+
// resolved rather than a fix failure. Require the
620+
// marker to stand alone so prose findings echoed
621+
// alongside it cannot silently close the review.
622+
if minSev != "" && config.IsMarkerOnlyOutput(output) {
623623
fmt.Println(
624624
"All findings below severity " +
625625
"threshold - closing review",

cmd/roborev/review.go

Lines changed: 28 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -26,21 +26,22 @@ const MaxDirtyDiffSize = 200 * 1024
2626

2727
func reviewCmd() *cobra.Command {
2828
var (
29-
repoPath string
30-
sha string
31-
agent string
32-
model string
33-
reasoning string
34-
reviewType string
35-
fast bool
36-
quiet bool
37-
dirty bool
38-
wait bool
39-
branch string
40-
baseBranch string
41-
since string
42-
local bool
43-
provider string
29+
repoPath string
30+
sha string
31+
agent string
32+
model string
33+
reasoning string
34+
reviewType string
35+
fast bool
36+
quiet bool
37+
dirty bool
38+
wait bool
39+
branch string
40+
baseBranch string
41+
since string
42+
local bool
43+
provider string
44+
minSeverity string
4445
)
4546

4647
cmd := &cobra.Command{
@@ -270,7 +271,7 @@ Examples:
270271

271272
// Handle --local mode: run agent directly without daemon
272273
if local {
273-
return runLocalReview(cmd, root, gitRef, diffContent, agent, model, provider, reasoning, reviewType, quiet)
274+
return runLocalReview(cmd, root, gitRef, diffContent, agent, model, provider, reasoning, reviewType, quiet, minSeverity)
274275
}
275276

276277
// Build request body
@@ -284,6 +285,7 @@ Examples:
284285
Reasoning: reasoning,
285286
ReviewType: reviewType,
286287
DiffContent: diffContent,
288+
MinSeverity: minSeverity,
287289
}
288290

289291
reqBody, _ := json.Marshal(reqFields)
@@ -358,14 +360,15 @@ Examples:
358360
cmd.Flags().BoolVar(&local, "local", false, "run review locally without daemon (streams output to console)")
359361
cmd.Flags().StringVar(&reviewType, "type", "", "review type (security, design) — changes system prompt")
360362
cmd.Flags().StringVar(&provider, "provider", "", "provider for pi agent (e.g. anthropic, openai)")
363+
cmd.Flags().StringVar(&minSeverity, "min-severity", "", "minimum severity threshold: critical, high, medium, low")
361364
registerAgentCompletion(cmd)
362365
registerReasoningCompletion(cmd)
363366

364367
return cmd
365368
}
366369

367370
// runLocalReview runs a review directly without the daemon
368-
func runLocalReview(cmd *cobra.Command, repoPath, gitRef, diffContent, agentName, model, provider, reasoning, reviewType string, quiet bool) error {
371+
func runLocalReview(cmd *cobra.Command, repoPath, gitRef, diffContent, agentName, model, provider, reasoning, reviewType string, quiet bool, minSeverity string) error {
369372
// Load config
370373
cfg, err := config.LoadGlobal()
371374
if err != nil {
@@ -378,6 +381,12 @@ func runLocalReview(cmd *cobra.Command, repoPath, gitRef, diffContent, agentName
378381
return fmt.Errorf("invalid reasoning: %w", err)
379382
}
380383

384+
// Resolve review min-severity
385+
resolvedMinSev, err := config.ResolveReviewMinSeverity(minSeverity, repoPath, cfg)
386+
if err != nil {
387+
return fmt.Errorf("invalid min-severity: %w", err)
388+
}
389+
381390
// Map review_type to config workflow (matches daemon behavior)
382391
workflow := "review"
383392
if !config.IsDefaultReviewType(reviewType) {
@@ -435,13 +444,13 @@ func runLocalReview(cmd *cobra.Command, repoPath, gitRef, diffContent, agentName
435444
var snapshotCleanup func()
436445
if diffContent != "" {
437446
// Dirty review
438-
dirtyResult, dirtyErr := pb.BuildDirtyWithSnapshot(repoPath, diffContent, 0, cfg.ReviewContextCount, a.Name(), reviewType)
447+
dirtyResult, dirtyErr := pb.BuildDirtyWithSnapshot(repoPath, diffContent, 0, cfg.ReviewContextCount, a.Name(), reviewType, resolvedMinSev)
439448
reviewPrompt = dirtyResult.Prompt
440449
snapshotCleanup = dirtyResult.Cleanup
441450
err = dirtyErr
442451
} else {
443452
excludes := config.ResolveExcludePatterns(repoPath, cfg, reviewType)
444-
result, buildErr := pb.BuildWithSnapshot(repoPath, gitRef, 0, cfg.ReviewContextCount, a.Name(), reviewType, excludes)
453+
result, buildErr := pb.BuildWithSnapshot(repoPath, gitRef, 0, cfg.ReviewContextCount, a.Name(), reviewType, resolvedMinSev, excludes)
445454
reviewPrompt = result.Prompt
446455
snapshotCleanup = result.Cleanup
447456
err = buildErr

internal/config/config.go

Lines changed: 109 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,11 @@ type Config struct {
144144
SecurityBackupModel string `toml:"security_backup_model"`
145145
DesignBackupModel string `toml:"design_backup_model"`
146146

147+
// Minimum severity thresholds (global defaults)
148+
ReviewMinSeverity string `toml:"review_min_severity" comment:"Minimum severity for reviews: critical, high, medium, or low. Empty disables filtering."`
149+
RefineMinSeverity string `toml:"refine_min_severity" comment:"Minimum severity for refine: critical, high, medium, or low. Empty disables filtering."`
150+
FixMinSeverity string `toml:"fix_min_severity" comment:"Minimum severity for fix: critical, high, medium, or low. Empty disables filtering."`
151+
147152
AllowUnsafeAgents *bool `toml:"allow_unsafe_agents"` // nil = not set, allows commands to choose their own default
148153
DisableCodexSandbox bool `toml:"disable_codex_sandbox"` // use --full-auto instead of --sandbox read-only (for systems where bwrap is broken)
149154
ReuseReviewSession *bool `toml:"reuse_review_session"` // nil = not set; when true, reuse prior branch review sessions when possible
@@ -621,8 +626,9 @@ type RepoConfig struct {
621626
ReviewReasoning string `toml:"review_reasoning" comment:"Reasoning level for reviews in this repo: fast, standard, medium, thorough, or maximum."`
622627
RefineReasoning string `toml:"refine_reasoning" comment:"Reasoning level for refine in this repo: fast, standard, medium, thorough, or maximum."`
623628
FixReasoning string `toml:"fix_reasoning" comment:"Reasoning level for fix in this repo: fast, standard, medium, thorough, or maximum."`
624-
FixMinSeverity string `toml:"fix_min_severity" comment:"Minimum severity for fix in this repo: critical, high, medium, or low."` // Minimum severity for fix: critical, high, medium, low
625-
RefineMinSeverity string `toml:"refine_min_severity" comment:"Minimum severity for refine in this repo: critical, high, medium, low."` // Minimum severity for refine: critical, high, medium, low
629+
FixMinSeverity string `toml:"fix_min_severity" comment:"Minimum severity for fix in this repo: critical, high, medium, or low."` // Minimum severity for fix: critical, high, medium, low
630+
RefineMinSeverity string `toml:"refine_min_severity" comment:"Minimum severity for refine in this repo: critical, high, medium, low."` // Minimum severity for refine: critical, high, medium, low
631+
ReviewMinSeverity string `toml:"review_min_severity" comment:"Minimum severity for reviews in this repo: critical, high, medium, low."` // Minimum severity for review: critical, high, medium, low
626632
ExcludePatterns []string `toml:"exclude_patterns" comment:"Filenames or glob patterns to exclude from review diffs for this repo."`
627633
PostCommitReview string `toml:"post_commit_review" comment:"Automatic post-commit review mode for this repo: commit or branch."` // "commit" (default) or "branch"
628634
ReuseReviewSession *bool `toml:"reuse_review_session"`
@@ -1442,6 +1448,54 @@ var severityAbove = map[string]string{
14421448
// from "agent couldn't fix it."
14431449
const SeverityThresholdMarker = "SEVERITY_THRESHOLD_MET"
14441450

1451+
// IsMarkerOnlyOutput reports whether output is essentially the
1452+
// SeverityThresholdMarker by itself, allowing only whitespace and
1453+
// minimal markdown decoration: bold (**...** or __...__), italic
1454+
// (*...* or _..._), a fenced code block, a leading list bullet,
1455+
// and an optional trailing period. Any prose or other substantive
1456+
// content disqualifies the output, since we cannot reliably tell
1457+
// chatty narration from prose findings without severity labels.
1458+
//
1459+
// Callers that want to treat the marker as a "below threshold"
1460+
// signal should use this helper rather than substring matching,
1461+
// which is too easy to fool with marker-bearing prose findings.
1462+
func IsMarkerOnlyOutput(output string) bool {
1463+
s := strings.TrimSpace(output)
1464+
if s == "" {
1465+
return false
1466+
}
1467+
1468+
// Strip a fenced code block if the output is wrapped in one.
1469+
if rest, ok := strings.CutPrefix(s, "```"); ok {
1470+
if _, after, found := strings.Cut(rest, "\n"); found {
1471+
s = after
1472+
} else {
1473+
s = rest
1474+
}
1475+
s = strings.TrimSuffix(s, "```")
1476+
s = strings.TrimSpace(s)
1477+
}
1478+
1479+
// Strip a leading list marker (- or *) followed by space.
1480+
if len(s) >= 2 && (s[0] == '-' || s[0] == '*') && s[1] == ' ' {
1481+
s = strings.TrimSpace(s[2:])
1482+
}
1483+
1484+
// Strip surrounding bold/italic markers. Bold forms (** and __)
1485+
// are stripped before italic forms (* and _) so that **X** fully
1486+
// unwraps in one pass rather than degrading to *X*.
1487+
for _, wrap := range []string{"**", "__", "*", "_"} {
1488+
if strings.HasPrefix(s, wrap) && strings.HasSuffix(s, wrap) && len(s) >= 2*len(wrap) {
1489+
s = strings.TrimSpace(s[len(wrap) : len(s)-len(wrap)])
1490+
}
1491+
}
1492+
1493+
// Strip a trailing period.
1494+
s = strings.TrimSpace(strings.TrimSuffix(s, "."))
1495+
1496+
return s == SeverityThresholdMarker
1497+
}
1498+
14451499
// SeverityInstruction returns a prompt instruction telling the agent
14461500
// to focus only on findings at or above minSeverity. Returns "" for
14471501
// empty, "low", or unrecognized input (no filtering needed).
@@ -1567,37 +1621,76 @@ func validateRepoReasoningOverride(
15671621
}
15681622

15691623
// ResolveFixMinSeverity determines minimum severity for fix.
1570-
// Priority: explicit > per-repo config > "" (no filter)
1571-
func ResolveFixMinSeverity(
1572-
explicit string, repoPath string,
1573-
) (string, error) {
1624+
// Priority: explicit > per-repo config > global config > "" (no filter)
1625+
func ResolveFixMinSeverity(explicit string, repoPath string, globalCfg *Config) (string, error) {
15741626
if strings.TrimSpace(explicit) != "" {
15751627
return NormalizeMinSeverity(explicit)
15761628
}
1577-
if repoCfg, err := LoadRepoConfig(repoPath); err == nil &&
1578-
repoCfg != nil &&
1579-
strings.TrimSpace(repoCfg.FixMinSeverity) != "" {
1629+
if repoCfg, err := LoadRepoConfig(repoPath); err == nil && repoCfg != nil && strings.TrimSpace(repoCfg.FixMinSeverity) != "" {
15801630
return NormalizeMinSeverity(repoCfg.FixMinSeverity)
15811631
}
1632+
if globalCfg != nil && strings.TrimSpace(globalCfg.FixMinSeverity) != "" {
1633+
return NormalizeMinSeverity(globalCfg.FixMinSeverity)
1634+
}
15821635
return "", nil
15831636
}
15841637

15851638
// ResolveRefineMinSeverity determines minimum severity for refine.
1586-
// Priority: explicit > per-repo config > "" (no filter)
1587-
func ResolveRefineMinSeverity(
1588-
explicit string, repoPath string,
1589-
) (string, error) {
1639+
// Priority: explicit > per-repo config > global config > "" (no filter)
1640+
func ResolveRefineMinSeverity(explicit string, repoPath string, globalCfg *Config) (string, error) {
15901641
if strings.TrimSpace(explicit) != "" {
15911642
return NormalizeMinSeverity(explicit)
15921643
}
1593-
if repoCfg, err := LoadRepoConfig(repoPath); err == nil &&
1594-
repoCfg != nil &&
1595-
strings.TrimSpace(repoCfg.RefineMinSeverity) != "" {
1644+
if repoCfg, err := LoadRepoConfig(repoPath); err == nil && repoCfg != nil && strings.TrimSpace(repoCfg.RefineMinSeverity) != "" {
15961645
return NormalizeMinSeverity(repoCfg.RefineMinSeverity)
15971646
}
1647+
if globalCfg != nil && strings.TrimSpace(globalCfg.RefineMinSeverity) != "" {
1648+
return NormalizeMinSeverity(globalCfg.RefineMinSeverity)
1649+
}
1650+
return "", nil
1651+
}
1652+
1653+
// ResolveReviewMinSeverity determines minimum severity for review.
1654+
// Priority: explicit > per-repo config > global config > "" (no filter)
1655+
func ResolveReviewMinSeverity(explicit string, repoPath string, globalCfg *Config) (string, error) {
1656+
if strings.TrimSpace(explicit) != "" {
1657+
return NormalizeMinSeverity(explicit)
1658+
}
1659+
if repoCfg, err := LoadRepoConfig(repoPath); err == nil && repoCfg != nil && strings.TrimSpace(repoCfg.ReviewMinSeverity) != "" {
1660+
return NormalizeMinSeverity(repoCfg.ReviewMinSeverity)
1661+
}
1662+
if globalCfg != nil && strings.TrimSpace(globalCfg.ReviewMinSeverity) != "" {
1663+
return NormalizeMinSeverity(globalCfg.ReviewMinSeverity)
1664+
}
15981665
return "", nil
15991666
}
16001667

1668+
// severityRank returns a numeric rank for a severity level.
1669+
// Higher rank = stricter threshold (fewer findings pass).
1670+
func severityRank(s string) int {
1671+
switch s {
1672+
case "critical":
1673+
return 4
1674+
case "high":
1675+
return 3
1676+
case "medium":
1677+
return 2
1678+
case "low":
1679+
return 1
1680+
default:
1681+
return 0
1682+
}
1683+
}
1684+
1685+
// StricterSeverity returns whichever severity threshold is stricter
1686+
// (filters more). Empty string means "no filter" (least strict).
1687+
func StricterSeverity(a, b string) string {
1688+
if severityRank(a) >= severityRank(b) {
1689+
return a
1690+
}
1691+
return b
1692+
}
1693+
16011694
// ResolveModel determines which model to use based on config priority:
16021695
// 1. Explicit model parameter (if non-empty)
16031696
// 2. Per-repo config (model in .roborev.toml)

0 commit comments

Comments
 (0)