diff --git a/README.md b/README.md index 19c0da4..77870e1 100644 --- a/README.md +++ b/README.md @@ -350,9 +350,13 @@ context (`GADFLY_REPO`, `GADFLY_PR`) automatically. Findings are extracted heuristically from each lens's markdown — a `path:line` reference anchors a finding, titled by the nearest preceding heading / numbered -item / bold lead-in. The emit is strictly best-effort: a short (~10s) timeout, -any error (or a non-2xx response) is logged to stderr only, and it **never** -changes the review output or the exit code. +item / bold lead-in. A lens whose verdict is **"No material issues found"** +emits **no** findings: its `path:line` references are verification notes +("verified X is safe"), not problems, so extracting them would record false +positives and unfairly penalize thorough clean-pass reviewers. The emit is +strictly best-effort: a short (~10s) timeout, any error (or a non-2xx response) +is logged to stderr only, and it **never** changes the review output or the exit +code. ## Building locally diff --git a/cmd/gadfly/emit.go b/cmd/gadfly/emit.go index 6e83fce..2cfc9c0 100644 --- a/cmd/gadfly/emit.go +++ b/cmd/gadfly/emit.go @@ -121,6 +121,14 @@ func emit(results []specialistResult, elapsed time.Duration) { if r.errored { continue // a failed lens contributes no findings } + // A lens that reports "No material issues found" has nothing to flag — + // its path:line references are verification notes ("verified X at + // file:line is safe"), not problems. Extracting them pollutes the + // findings store with false positives and unfairly penalizes thorough + // reviewers that do clean passes, so a clean lens emits no findings. + if r.verdict == verdictClean { + continue + } sev := r.verdict.label() for _, f := range parseFindings(r.spec, r.out) { reports = append(reports, reportPayload{ diff --git a/cmd/gadfly/emit_test.go b/cmd/gadfly/emit_test.go index a9fca88..fe1b29b 100644 --- a/cmd/gadfly/emit_test.go +++ b/cmd/gadfly/emit_test.go @@ -179,6 +179,46 @@ func TestEmit_PostsRunsAndReports(t *testing.T) { } } +// A clean "No material issues found" lens must NOT emit findings, even though +// its markdown contains path:line references (those are verification notes, not +// problems). /runs is still posted; /reports is not. +func TestEmit_SkipsCleanVerdictLens(t *testing.T) { + var runs, reports int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/runs": + atomic.AddInt32(&runs, 1) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"run_id":"x"}`)) + case "/reports": + atomic.AddInt32(&reports, 1) + w.WriteHeader(http.StatusOK) + default: + http.Error(w, "not found", http.StatusNotFound) + } + })) + defer srv.Close() + + t.Setenv("GADFLY_FINDINGS_URL", srv.URL) + t.Setenv("GADFLY_REPO", "owner/repo") + t.Setenv("GADFLY_PR", "7") + t.Setenv("GADFLY_MODEL", "ollama-cloud/qwen3") + + // Clean verdict, but the markdown is full of path:line "verified X" notes. + cleanMarkdown := "No material issues found.\n\nVerified `run/executor.go:166` handles the error.\n" + results := []specialistResult{ + {spec: Specialist{Name: "security"}, out: cleanMarkdown, verdict: verdictClean}, + } + emit(results, time.Second) + + if got := atomic.LoadInt32(&runs); got != 1 { + t.Fatalf("/runs received %d times, want 1 (the run is still recorded)", got) + } + if got := atomic.LoadInt32(&reports); got != 0 { + t.Fatalf("/reports received %d times, want 0 (clean lens emits no findings)", got) + } +} + func TestEmit_DisabledMakesNoRequests(t *testing.T) { var hits int32 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {