Claude (review-harness setup) 305f5b9ac0 Apply B1+B2+B3+B4 from 2026-04-30 cross-lineage scrum
Cross-lineage scrum (Opus 4.7 / Kimi K2.6 / Qwen3-coder via chatd's
/v1/chat) on the harness's first 4 commits surfaced 5 real bugs;
this commit lands the 4 inside the LLM/validator stack. B5 (scanner
skip-list semantics) ships separately as it changes scan behavior
on every target repo.

B1 (Kimi BLOCK + Opus WARN convergent) — internal/validators:
evidencePresent had two flaws: (1) cursor advanced on match in the
trim-line fallback, breaking same-line repeated matches AND skipping
not-yet-considered lines so out-of-order evidence spuriously failed;
(2) strings.Contains on a single `}` trim-matched any closing brace
in the file, defeating the "evidence quotes real text" contract.
Fix: trivial-evidence guard FIRST (reject anything <4 non-whitespace
chars) + per-line search no longer advances a cursor. New regression
test TestEvidencePresent_RejectsTrivialMatches covers `}`, `{`, `)`,
empty, and out-of-order multi-line evidence (which now passes —
order isn't part of the contract).

B2 (Kimi WARN + Opus WARN convergent) — internal/pipeline:
WriteJSON error for rejected-findings.json was swallowed with
`if err == nil`, so a write failure left the validation phase
reporting status="ok" while the audit trail vanished. Mirror the
validated-findings branch: surface the error in
validatePhase.Errors + bump status to degraded + ExitCode=66.

B3 (Kimi BLOCK + Opus BLOCK convergent) — internal/llm/ollama.go:
HealthCheck.basic_prompt_ok was set to true on ANY non-empty
response, so a model emitting `<think>...` traces or apologies
passed silently. Now requires the response to contain "OK"
(uppercase, substring). Substring rather than equality lets minor
whitespace/punctuation variations through (some models add a
trailing period). Errors now record what the model actually said
when it fails the check.

B4 (Opus BLOCK only — same class as today's chatd Anthropic-temp
fix) — internal/llm/ollama.go: chatBody had `if opts.Temperature != 0`
which silently dropped Temperature=0 from the request, so HealthCheck
+ Reviewer (both pass Temperature=0 expecting determinism) actually
ran at Ollama's ~0.8 default. Always forward Temperature now. The
two callers always set explicit values, so "0 means 0" is correct;
if a future caller wants Ollama's default they'll switch
CompleteOptions.Temperature to *float64 like chatd did this morning.

Verified end-to-end: insecure-repo + --enable-llm still produces 25
confirmed findings (16 static + 9 LLM), 0 rejected. Validator unit
tests: 11 pass (added TestEvidencePresent_RejectsTrivialMatches).

Same-day-as-shipping scrum, same-day-as-shipping fixes. The
convergent-≥2 gate caught 3 of these; the 4th was Opus-only but
verified by reading the code (same idiom as today's chatd bug).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-30 01:33:01 -05:00

258 lines
8.0 KiB
Go

package validators
import (
"os"
"path/filepath"
"testing"
"local-review-harness/internal/analyzers"
)
// makeFile is a tiny helper for test setup: write content to a path
// under tmp + return the abs path.
func makeFile(t *testing.T, dir, name, content string) string {
t.Helper()
p := filepath.Join(dir, name)
if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(p, []byte(content), 0o644); err != nil {
t.Fatal(err)
}
return p
}
// llmFinding builds a minimal Source=LLM finding.
func llmFinding(file, lineHint, evidence string) analyzers.Finding {
return analyzers.Finding{
Title: "test",
Severity: analyzers.SeverityHigh,
Status: analyzers.StatusSuspected,
File: file,
LineHint: lineHint,
Evidence: evidence,
Reason: "test reason",
Source: analyzers.SourceLLM,
}
}
// === GATE D1 — synthetic LLM finding citing nonexistent file → must reject ===
func TestValidate_RejectsNonexistentFile(t *testing.T) {
repo := t.TempDir()
out := Validate(repo, []analyzers.Finding{
llmFinding("does/not/exist.go", "42", "fmt.Println"),
})
if len(out.Rejected) != 1 {
t.Fatalf("expected 1 rejected finding, got %d", len(out.Rejected))
}
if out.Rejected[0].RejectionReason != ReasonFileNotFound {
t.Errorf("expected ReasonFileNotFound, got %q", out.Rejected[0].RejectionReason)
}
}
func TestValidate_RejectsEvidenceNotInFile(t *testing.T) {
repo := t.TempDir()
makeFile(t, repo, "real.go", "package main\nfunc main() {}\n")
out := Validate(repo, []analyzers.Finding{
llmFinding("real.go", "1", "this string does not exist in the file"),
})
if len(out.Rejected) != 1 {
t.Fatalf("expected 1 rejected, got %d", len(out.Rejected))
}
if out.Rejected[0].RejectionReason != ReasonNoEvidence {
t.Errorf("expected ReasonNoEvidence, got %q", out.Rejected[0].RejectionReason)
}
}
func TestValidate_RejectsLineHintBeyondFile(t *testing.T) {
repo := t.TempDir()
makeFile(t, repo, "small.go", "line one\nline two\n") // 2 lines
out := Validate(repo, []analyzers.Finding{
llmFinding("small.go", "100", "line one"),
})
if len(out.Rejected) != 1 {
t.Fatalf("expected 1 rejected, got %d", len(out.Rejected))
}
if out.Rejected[0].RejectionReason != ReasonLineHintTooHigh {
t.Errorf("expected ReasonLineHintTooHigh, got %q", out.Rejected[0].RejectionReason)
}
}
func TestValidate_AcceptsRealFinding(t *testing.T) {
repo := t.TempDir()
makeFile(t, repo, "good.go", "package main\nfunc badPattern() {}\n")
out := Validate(repo, []analyzers.Finding{
llmFinding("good.go", "2", "func badPattern()"),
})
if len(out.Validated) != 1 {
t.Fatalf("expected 1 validated, got %d (rejected=%d)", len(out.Validated), len(out.Rejected))
}
if out.Validated[0].Status != analyzers.StatusConfirmed {
t.Errorf("expected status=confirmed, got %q", out.Validated[0].Status)
}
}
func TestValidate_AcceptsEvidenceWithDifferentLeadingWhitespace(t *testing.T) {
// Models often re-indent code when quoting; the validator's
// trim-line-by-line fallback should accept it.
repo := t.TempDir()
makeFile(t, repo, "indented.go", "package main\n\n\tfunc indented() {\n\t\treturn\n\t}\n")
out := Validate(repo, []analyzers.Finding{
llmFinding("indented.go", "3", "func indented() {"), // model dropped leading tab
})
if len(out.Validated) != 1 {
t.Fatalf("expected 1 validated; got rejected=%d (reason=%q)",
len(out.Rejected),
func() Reason {
if len(out.Rejected) > 0 {
return out.Rejected[0].RejectionReason
}
return ""
}(),
)
}
}
func TestValidate_RejectsEmptyEvidence(t *testing.T) {
repo := t.TempDir()
makeFile(t, repo, "any.go", "package main\n")
out := Validate(repo, []analyzers.Finding{
llmFinding("any.go", "1", ""),
})
if len(out.Rejected) != 1 || out.Rejected[0].RejectionReason != ReasonEmptyEvidence {
t.Errorf("empty-evidence finding should be rejected with ReasonEmptyEvidence; got %+v", out.Rejected)
}
}
func TestValidate_PassesThroughStaticFindings(t *testing.T) {
// Static findings already have grep-able evidence by construction.
// Validator promotes them to confirmed without re-checking.
repo := t.TempDir()
staticF := analyzers.Finding{
Title: "static finding",
Severity: analyzers.SeverityMedium,
Status: analyzers.StatusSuspected,
File: "anywhere.go",
Evidence: "anything",
Source: analyzers.SourceStatic,
}
out := Validate(repo, []analyzers.Finding{staticF})
if len(out.Validated) != 1 {
t.Fatalf("static finding should pass through validated; got %d", len(out.Validated))
}
if out.Validated[0].Status != analyzers.StatusConfirmed {
t.Errorf("static finding should be promoted to confirmed; got %q", out.Validated[0].Status)
}
}
// TestValidate_AcceptsRelativeRepoPath locks in the fix for the
// 2026-04-30 bug where every real finding was rejected as
// file_not_found because the path-traversal check compared a
// relative joined path against an absolute repoAbs (HasPrefix
// always false). Caught by J running ./review-harness with a
// relative target path; gate D1 now exercises this path.
func TestValidate_AcceptsRelativeRepoPath(t *testing.T) {
repo := t.TempDir()
makeFile(t, repo, "src/handler.go", "package main\nfunc bad() {}\n")
// Pass the repo as a RELATIVE path. Pre-fix this triggered the bug.
cwd, _ := os.Getwd()
defer os.Chdir(cwd)
parent := filepath.Dir(repo)
relRepo, err := filepath.Rel(parent, repo)
if err != nil {
t.Skip("can't compute relative path: " + err.Error())
}
if err := os.Chdir(parent); err != nil {
t.Fatal(err)
}
out := Validate(relRepo, []analyzers.Finding{
llmFinding("src/handler.go", "2", "func bad()"),
})
if len(out.Validated) != 1 {
t.Errorf("relative repo path should still validate; got rejected=%d (reasons: %v)",
len(out.Rejected),
func() []Reason {
rs := []Reason{}
for _, r := range out.Rejected {
rs = append(rs, r.RejectionReason)
}
return rs
}())
}
}
// TestEvidencePresent_RejectsTrivialMatches locks in scrum fix B1.
// Pre-fix, evidence "}" or "{" trim-matched almost any line in the
// file via strings.Contains, so an LLM emitting a single brace would
// pass validation against any file with braces.
func TestEvidencePresent_RejectsTrivialMatches(t *testing.T) {
content := "package main\nfunc main() {\n\tfmt.Println(\"hi\")\n}\n"
cases := []struct {
evidence string
want bool
}{
// Verbatim full content matches
{"func main() {", true},
{"fmt.Println(\"hi\")", true},
// Trivial single-char/-line matches must REJECT
{"}", false},
{"{", false},
{")", false},
// Empty evidence is rejected (caller-side enforced too)
{"", false},
// Multi-line evidence in different order — must still pass
// (cursor-reset fix B1 — pre-fix this would fail because the
// cursor advanced past the first match)
{"fmt.Println(\"hi\")\nfunc main() {", true},
}
for _, c := range cases {
got := evidencePresent(content, c.evidence)
if got != c.want {
t.Errorf("evidencePresent(%q) = %v; want %v", c.evidence, got, c.want)
}
}
}
// === Path-traversal protection: hallucinated "../../../etc/passwd" must reject ===
func TestValidate_RejectsPathEscapingRepo(t *testing.T) {
repo := t.TempDir()
out := Validate(repo, []analyzers.Finding{
llmFinding("../../../etc/passwd", "1", "root:x:0:0"),
})
if len(out.Rejected) != 1 {
t.Fatalf("expected path-traversal finding rejected; got %d rejected", len(out.Rejected))
}
if out.Rejected[0].RejectionReason != ReasonFileNotFound {
t.Errorf("expected ReasonFileNotFound for path escape, got %q", out.Rejected[0].RejectionReason)
}
}
// === highestLine extractor ===
func TestHighestLine(t *testing.T) {
cases := []struct {
hint string
want int
ok bool
}{
{"42", 42, true},
{"10-20", 20, true},
{"line 100", 100, true},
{"L42", 42, true},
{"42:5", 42, true},
{"", 0, false},
{"none", 0, false},
{"15-30-50", 50, true}, // pick the largest
}
for _, c := range cases {
got, ok := highestLine(c.hint)
if got != c.want || ok != c.ok {
t.Errorf("highestLine(%q) = (%d, %v); want (%d, %v)", c.hint, got, ok, c.want, c.ok)
}
}
}