zzet--gortex/internal/review/critique_test.go

package review

import (
	"context"
	"errors"
	"strings"
	"testing"

	"github.com/stretchr/testify/require"
)

// critiqueFindings is a small fixture of three findings the critique pass
// adjudicates: a real error, a warning, and an info-level note.
func critiqueFindings() []Finding {
	return []Finding{
		{Rule: "nil-deref", Severity: SevError, Category: "nil-deref", File: "app/svc.go", Line: 12,
			Message: "p may be nil", SourceLine: "return p.Balance"},
		{Rule: "style-nit", Severity: SevWarning, Category: "style", File: "app/svc.go", Line: 20,
			Message: "prefer fmt.Errorf"},
		{Rule: "doc-todo", Severity: SevInfo, Category: "doc", File: "app/util.go", Line: 4,
			Message: "missing doc comment"},
	}
}

// TestCritiqueDropsOneFinding drives the happy path: a stubbed critique LLM marks
// the middle finding a false positive. The pass removes exactly that finding,
// records it in Dropped with its reason, keeps the other two, and recomputes the
// verdict over the kept set.
func TestCritiqueDropsOneFinding(t *testing.T) {
	findings := critiqueFindings()
	stub := &stubGen{reply: `[
		{"index":0,"verdict":"keep","reason":"genuine nil deref"},
		{"index":1,"verdict":"drop","reason":"style nit, not a defect"},
		{"index":2,"verdict":"keep","reason":"valid doc gap"}
	]`}

	res := Critique(context.Background(), stub.gen, findings, sampleDiff, 0)

	require.True(t, res.LLMUsed)
	require.Len(t, res.Kept, 2, "the dropped finding must be removed from the kept set")
	require.Len(t, res.Dropped, 1)
	require.Equal(t, "style-nit", res.Dropped[0].Finding.Rule)
	require.Equal(t, CritiqueDrop, res.Dropped[0].Verdict)
	require.Equal(t, "style nit, not a defect", res.Dropped[0].Reason)

	// The two surviving findings are the nil-deref and the doc-todo.
	keptRules := map[string]bool{}
	for _, f := range res.Kept {
		keptRules[f.Rule] = true
	}
	require.True(t, keptRules["nil-deref"])
	require.True(t, keptRules["doc-todo"])
	require.False(t, keptRules["style-nit"])

	// An error survived, so the verdict stays BLOCK.
	require.Equal(t, VerdictBlock, res.Verdict)
	require.Contains(t, res.Summary, "dropped 1")
}

// TestCritiqueVerdictDowngrade proves dropping the only error finding honestly
// downgrades the worst-of verdict over the kept set.
func TestCritiqueVerdictDowngrade(t *testing.T) {
	findings := critiqueFindings()
	stub := &stubGen{reply: `[
		{"index":0,"verdict":"drop","reason":"p is checked above the diff window"},
		{"index":1,"verdict":"keep","reason":""},
		{"index":2,"verdict":"keep","reason":""}
	]`}

	res := Critique(context.Background(), stub.gen, findings, "", 0)

	require.Len(t, res.Dropped, 1)
	require.Equal(t, "nil-deref", res.Dropped[0].Finding.Rule)
	// With the error dropped, the worst remaining is a warning → REVIEW.
	require.Equal(t, VerdictReview, res.Verdict)
}

// TestCritiqueDisabledIsNoOp proves a nil gen is a pass-through: every finding is
// kept, nothing dropped, no error, LLMUsed false.
func TestCritiqueDisabledIsNoOp(t *testing.T) {
	findings := critiqueFindings()

	res := Critique(context.Background(), nil, findings, sampleDiff, 0)

	require.False(t, res.LLMUsed)
	require.Len(t, res.Kept, len(findings))
	require.Empty(t, res.Dropped)
	require.Equal(t, VerdictBlock, res.Verdict) // unchanged worst-of
	require.Contains(t, res.Summary, "kept all")
}

// TestCritiqueGarbageKeepsAll proves an unparseable or non-JSON model response is
// treated as a no-op pass-through rather than an error or a finding wipe.
func TestCritiqueGarbageKeepsAll(t *testing.T) {
	findings := critiqueFindings()
	for _, reply := range []string{
		"I think these all look fine to me, no JSON here.",
		"```\nnot json\n```",
		"",
	} {
		stub := &stubGen{reply: reply}
		res := Critique(context.Background(), stub.gen, findings, sampleDiff, 0)
		require.False(t, res.LLMUsed, "reply %q should not count as an LLM-driven critique", reply)
		require.Len(t, res.Kept, len(findings), "garbage reply %q must keep every finding", reply)
		require.Empty(t, res.Dropped)
	}
}

// TestCritiqueModelErrorKeepsAll proves a failing model degrades to pass-through.
func TestCritiqueModelErrorKeepsAll(t *testing.T) {
	findings := critiqueFindings()
	stub := &stubGen{err: errors.New("provider unavailable")}

	res := Critique(context.Background(), stub.gen, findings, sampleDiff, 0)

	require.False(t, res.LLMUsed)
	require.Len(t, res.Kept, len(findings))
	require.Empty(t, res.Dropped)
}

// TestCritiqueUnknownVerdictKeeps proves an unrecognised verdict token, an
// unadjudicated finding (no row for its index), and an "uncertain" verdict all
// keep the finding — only an explicit "drop" removes one.
func TestCritiqueUnknownVerdictKeeps(t *testing.T) {
	findings := critiqueFindings()
	// index 0 → gibberish verdict (keep), index 1 → uncertain (keep + counted),
	// index 2 → no row at all (default keep).
	stub := &stubGen{reply: `[
		{"index":0,"verdict":"probably-fine","reason":"who knows"},
		{"index":1,"verdict":"uncertain","reason":"cannot tell from the diff"}
	]`}

	res := Critique(context.Background(), stub.gen, findings, sampleDiff, 0)

	require.True(t, res.LLMUsed)
	require.Len(t, res.Kept, 3, "no explicit drop → keep everything")
	require.Empty(t, res.Dropped)
	require.Equal(t, 1, res.Uncertain)
}

// TestCritiqueExtractsArrayFromProse proves the parser slices the JSON array out
// of a response wrapped in prose / code fences and ignores brackets inside reason
// strings.
func TestCritiqueExtractsArrayFromProse(t *testing.T) {
	findings := critiqueFindings()
	stub := &stubGen{reply: "Sure, here is my critique:\n```json\n[" +
		`{"index":1,"verdict":"drop","reason":"the [style] nit is cosmetic"}` +
		"]\n```\nHope that helps!"}

	res := Critique(context.Background(), stub.gen, findings, sampleDiff, 0)

	require.True(t, res.LLMUsed)
	require.Len(t, res.Dropped, 1)
	require.Equal(t, "style-nit", res.Dropped[0].Finding.Rule)
	require.Equal(t, "the [style] nit is cosmetic", res.Dropped[0].Reason)
}

// TestBuildCritiquePromptGrounding proves the prompt carries the diff grounding,
// each numbered finding, and its flagged source line.
func TestBuildCritiquePromptGrounding(t *testing.T) {
	findings := critiqueFindings()
	prompt := BuildCritiquePrompt(findings, sampleDiff)

	require.Contains(t, prompt, "SECOND PASS")
	require.Contains(t, prompt, "CHANGESET DIFF")
	require.Contains(t, prompt, "func Risky") // from sampleDiff
	require.Contains(t, prompt, "[0]")
	require.Contains(t, prompt, "rule=nil-deref")
	require.Contains(t, prompt, "flagged source: return p.Balance")
	require.Contains(t, prompt, `"verdict":"keep|drop|uncertain"`)
}

// TestCritiqueEmptyFindings proves a zero-finding input is a clean no-op.
func TestCritiqueEmptyFindings(t *testing.T) {
	stub := &stubGen{reply: `[]`}
	res := Critique(context.Background(), stub.gen, nil, sampleDiff, 0)
	require.False(t, res.LLMUsed)
	require.Empty(t, res.Kept)
	require.Empty(t, res.Dropped)
	require.Equal(t, VerdictApprove, res.Verdict)
	require.NotContains(t, strings.ToLower(res.Summary), "dropped")
}