fix: validate consistency findings and retain partial review coverage

docs/changes/unreleased/1820-review-validity-partial-coverage.md

@@ -0,0 +1,14 @@
+---
+kind: fixed
+title: review rejects malformed consistency claims and retains partial coverage
+pr: 1820
+surface: [chat, engine, docs]
+invalidates:
+  - "A negative consistency claim could omit its explanation and code location and still become a finding. Such output now ends with a non-postable failure record, and posting refuses malformed older findings."
+  - "A partial run warned in its account but saved JSON omitted the coverage facts and its posting payload could still approve or recommend a clean merge. New reports persist their limits, keep the warning in the posting payload and avoid those claims."
+---
+
+Assembled partial reviews still finish successfully. Invalid consistency claims do
+not. Models and limits are unchanged; older reports with known partial metadata
+need coverage facts and a posting warning before posting. This repair does not
+resolve the remaining lens session deadlines or establish complete live acceptance.

internal/manual/chat/review.md

@@ -66,6 +66,20 @@ When it finishes, the chat is handed a short account whose first line says what
 
 A shell run's record folder is under `~/.codeaf/v3/carried/review/`, one folder per run.
 
+An assembled review can finish successfully while covering only part of the change.
+Its first line says `Partial review` when a session failed, a dimension degraded,
+or a time or dollar limit stopped work. The JSON report keeps those coverage facts,
+and the review it would post carries the same warning. A partial review cannot be
+posted as an approval; a valid partial report can still be posted as a comment or
+request for changes on your yes. Terminal success means the report was assembled,
+not that every check completed.
+
+A reported consistency defect must include its explanation and code
+location with a valid line range. Missing details end the run with a failure record
+instead of a postable review. Posting also refuses such malformed findings in older
+saved reports. Older reports with a known budget stop or degraded work need the
+coverage facts and posting warning before they can be posted.
+
 If the pipeline stops before it returns an assembled review, the run keeps `review-failure.json` and `review-failure.md` instead. They record the stage, reason, elapsed time, recorded spend and failed call counts. They contain no review and cannot be posted as one; the run still ends as failed or stopped on its ceiling.
 
 ## Post the review to GitHub — comment on the pull request, request changes, /review post

internal/praf/orch/consistency_validity_test.go

@@ -0,0 +1,49 @@
+package orch
+
+import (
+	"context"
+	"testing"
+
+	"github.com/Agent-Field/codeaf/internal/praf/reasoners"
+	"github.com/Agent-Field/codeaf/internal/praf/schemas"
+)
+
+func TestConsistencyVerdictsRequireFindingDetailsOnlyWhenTheyClaimADefect(t *testing.T) {
+	for _, tc := range []struct {
+		name    string
+		change  func(map[string]any)
+		invalid bool
+		count   int
+	}{
+		{"grounded defect", func(map[string]any) {}, false, 1},
+		{"property holds", func(v map[string]any) { v["holds"] = true; delete(v, "body"); delete(v, "file_path") }, false, 0},
+		{"live draft without details", func(v map[string]any) {
+			v["title"] = "exploratory internal note: property holds; holds=true"
+			delete(v, "body")
+			delete(v, "file_path")
+			delete(v, "line_start")
+			delete(v, "line_end")
+		}, true, 0},
+		{"blank explanation", func(v map[string]any) { v["body"] = " \n" }, true, 0},
+		{"missing location", func(v map[string]any) { v["file_path"] = " " }, true, 0},
+		{"missing line", func(v map[string]any) { v["line_start"] = 0 }, true, 0},
+		{"reversed lines", func(v map[string]any) { v["line_end"] = 1 }, true, 0},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			o := degradationOrchestrator(t)
+			o.prData.ChangedFiles = []schemas.ChangedFile{{Path: "a.go", Patch: "@@ -1 +1 @@\n-old\n+new"}}
+			o.rfns.extractOblig = func(context.Context, reasoners.Deps, reasoners.ExtractObligationsInput) (map[string]any, error) {
+				return map[string]any{"obligations": []any{map[string]any{"property": "keys agree"}}}, nil
+			}
+			o.rfns.verifyOblig = func(context.Context, reasoners.Deps, reasoners.VerifyObligationInput) (map[string]any, error) {
+				v := map[string]any{"holds": false, "title": "keys disagree", "body": "Admission and completion use different keys.", "file_path": "a.go", "line_start": 3, "line_end": 4}
+				tc.change(v)
+				return v, nil
+			}
+			findings, err := o.runConsistencyVerify(context.Background(), nil)
+			if (err != nil) != tc.invalid || len(findings) != tc.count {
+				t.Fatalf("findings=%+v, err=%v; invalid=%v count=%d", findings, err, tc.invalid, tc.count)
+			}
+		})
+	}
+}

internal/praf/orch/output.go

@@ -38,6 +38,11 @@ func (o *Orchestrator) generateOutput(
 	if o.prData == nil {
 		return schemas.ReviewResult{}, errPRDataNotInitialized
 	}
+	for _, finding := range scoredFindings {
+		if err := finding.ValidateConsistency(); err != nil {
+			return schemas.ReviewResult{}, err
+		}
+	}
 
 	diffFiles := map[string]struct{}{}
 	for _, cf := range o.prData.ChangedFiles {
@@ -334,6 +339,24 @@ func (o *Orchestrator) formatCommentBody(finding schemas.ScoredFinding) string {
 	return strings.TrimSpace(strings.Join(lines, "\n"))
 }
 
+const advisoryCallout = "> [!NOTE]\n> **Advisory — non-blocking.** Safe to merge and address in a follow-up."
+
+// PartialReviewPayload keeps findings available without a clean merge claim.
+// Its body is built from completed work by the host that knows what stopped.
+func PartialReviewPayload(review schemas.GitHubReview, body string) schemas.GitHubReview {
+	review.Body = body
+	if review.Event == "APPROVE" {
+		review.Event = "COMMENT"
+	}
+	comments := make([]schemas.GitHubComment, len(review.Comments))
+	for i, comment := range review.Comments {
+		comment.Body = strings.Replace(comment.Body, advisoryCallout, "> [!NOTE]\n> **Advisory — non-blocking.** This finding is from a partial review.", 1)
+		comments[i] = comment
+	}
+	review.Comments = comments
+	return review
+}
+
 func decorateWithBlocking(finding *schemas.ScoredFinding, body string) string {
 	var header string
 	if finding != nil && finding.Blocking {
@@ -342,7 +365,7 @@ func decorateWithBlocking(finding *schemas.ScoredFinding, body string) string {
 			header += " " + finding.BlockingReason
 		}
 	} else {
-		header = "> [!NOTE]\n> **Advisory — non-blocking.** Safe to merge and address in a follow-up."
+		header = advisoryCallout
 		if finding != nil && finding.BlockingReason != "" {
 			header += "\n>\n> _Why non-blocking:_ " + finding.BlockingReason
 		}

internal/praf/orch/output_test.go

@@ -37,6 +37,19 @@ func fixedClockOrch(t *testing.T) *Orchestrator {
 	return o
 }
 
+func TestPartialPayloadPreservesCommentsWithoutTheCleanAdvisoryCallout(t *testing.T) {
+	finding := schemas.ScoredFinding{Title: "Fix nil lookup", Body: "Dereference follows failure.", Severity: "important"}
+	body := decorateWithBlocking(&finding, "**Fix nil lookup**\n\nDereference follows failure.")
+	original := schemas.GitHubReview{Body: "Safe to merge", Event: "COMMENT", Comments: []schemas.GitHubComment{{Path: "a.go", Line: 3, Body: body}}}
+	partial := PartialReviewPayload(original, "Partial review; one session failed.")
+	if strings.Contains(partial.Body, "Safe to merge") || strings.Contains(partial.Comments[0].Body, "Safe to merge") || !strings.Contains(partial.Comments[0].Body, "Dereference follows failure.") || partial.Comments[0].Path != "a.go" || partial.Comments[0].Line != 3 {
+		t.Fatalf("partial payload=%+v", partial)
+	}
+	if original.Comments[0].Body != body {
+		t.Fatal("partial conversion mutated the original payload")
+	}
+}
+
 type ownPRFallbackGH struct {
 	reviews []schemas.GitHubReview
 }

internal/praf/orch/phases.go

@@ -1073,7 +1073,7 @@ func (o *Orchestrator) runConsistencyVerify(ctx context.Context, allFindings []s
 			continue
 		}
 		holds, present := v["holds"]
-		if !present || holds != false || getStr(v, "title", "") == "" {
+		if !present || holds != false {
 			continue
 		}
 		lineEndVal := getIntOrZero(v, "line_end")
@@ -1094,7 +1094,11 @@ func (o *Orchestrator) runConsistencyVerify(ctx context.Context, allFindings []s
 			"confidence":     getFloatOr(v, "confidence", 0.7),
 			"tags":           []string{"consistency"},
 		}
-		newFindings = append(newFindings, mapToStruct[schemas.ReviewFinding](normalized))
+		finding := mapToStruct[schemas.ReviewFinding](normalized)
+		if err := finding.ValidateConsistency(); err != nil {
+			return nil, err
+		}
+		newFindings = append(newFindings, finding)
 	}
 	return append(allFindings, newFindings...), nil
 }

internal/praf/post.go

@@ -52,6 +52,24 @@ func readSaved(path string) (Saved, string, error) {
 	case strings.TrimSpace(saved.Review.Review.Body) == "" && len(saved.Review.Review.Comments) == 0:
 		return Saved{}, path, fmt.Errorf("the review at %s has nothing to post", path)
 	}
+	for _, finding := range saved.Review.Findings {
+		if err := finding.ValidateConsistency(); err != nil {
+			return Saved{}, path, err
+		}
+	}
+	if saved.Coverage == nil && (saved.Review.Summary.BudgetExhausted || saved.Review.Metadata.DegradedDimensions > 0) {
+		return Saved{}, path, errors.New("the older partial review does not keep coverage facts and a posting warning")
+	}
+	if coverage := saved.Coverage; coverage != nil {
+		complete := coverage.FailedSessions == 0 && coverage.FailedCalls == 0 && coverage.DegradedDimensions == 0 && !coverage.BudgetExhausted && coverage.StoppedAt == ""
+		if coverage.FailedSessions < 0 || coverage.FailedCalls < 0 || coverage.DegradedDimensions < 0 || coverage.Complete != complete ||
+			coverage.BudgetExhausted != saved.Review.Summary.BudgetExhausted || coverage.DegradedDimensions != saved.Review.Metadata.DegradedDimensions {
+			return Saved{}, path, errors.New("the review has inconsistent coverage facts")
+		}
+		if !complete && (saved.Review.Review.Event == "APPROVE" || strings.TrimSpace(coverage.Note) == "" || !strings.HasPrefix(saved.Review.Review.Body, partialWarning+" "+coverage.Note)) {
+			return Saved{}, path, errors.New("a partial review must keep its coverage warning and cannot be posted as an approval")
+		}
+	}
 	return saved, path, nil
 }
 

internal/praf/report.go

@@ -40,8 +40,23 @@ type Saved struct {
 	PullRequest SavedPullRequest     `json:"pull_request"`
 	Focus       string               `json:"focus,omitempty"`
 	Review      schemas.ReviewResult `json:"review"`
+	Coverage    *ReportCoverage      `json:"coverage,omitempty"`
 }
 
+// Coverage is separate from execution status: an assembled partial review can
+// end successfully, but a saved or posted report must keep its missing work.
+type ReportCoverage struct {
+	Complete           bool   `json:"complete"`
+	FailedSessions     int    `json:"failed_sessions"`
+	FailedCalls        int    `json:"failed_calls"`
+	DegradedDimensions int    `json:"degraded_dimensions"`
+	BudgetExhausted    bool   `json:"budget_exhausted"`
+	StoppedAt          string `json:"stopped_at,omitempty"`
+	Note               string `json:"note,omitempty"`
+}
+
+const partialWarning = "**Partial review.**"
+
 // SavedPullRequest is the pull request a saved review is of.
 type SavedPullRequest struct {
 	Owner   string `json:"owner"`
@@ -69,6 +84,7 @@ type runFacts struct {
 	cutAt    string
 	wall     string
 	failNote string
+	partial  bool
 }
 
 // writeReport writes the full report into folder and answers the paths it
@@ -149,6 +165,9 @@ func account(review schemas.ReviewResult, run runFacts, files []string) string {
 // review found in counts, blocking first.
 func outcomeLine(review schemas.ReviewResult, run runFacts) string {
 	what := "Review of " + run.target.String()
+	if run.partial {
+		what = "Partial review of " + run.target.String()
+	}
 	if title := oneLine(run.title); title != "" {
 		what += " (" + title + ")"
 	}

internal/praf/report_validity_test.go

@@ -0,0 +1,156 @@
+package praf
+
+import (
+	"context"
+	"encoding/json"
+	"io"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/Agent-Field/codeaf/internal/delegate"
+	"github.com/Agent-Field/codeaf/internal/praf/orch"
+	"github.com/Agent-Field/codeaf/internal/praf/schemas"
+)
+
+func invalidConsistencyResult() schemas.ReviewResult {
+	r := sampleResult()
+	r.Findings = []schemas.ScoredFinding{{DimensionID: "consistency-verify", Title: "draft note: the property holds; holds=true", Severity: "important"}}
+	return r
+}
+
+func TestInvalidConsistencyOutputCannotBecomeASavedOrPostedReview(t *testing.T) {
+	r, _ := testReviewer(&fakeSessions{}, func(context.Context, orch.Deps) (schemas.ReviewResult, error) { return invalidConsistencyResult(), nil })
+	host := newTestHost(t)
+	end := runReview(context.Background(), host, ReadBrief("o/r#7", nil), defaults(), io.Discard, r)
+	if end.Status != delegate.StatusFail {
+		t.Fatalf("invalid output ended %s: %s", end.Status, end.Deliverable)
+	}
+	if _, err := os.Stat(filepath.Join(host.records, reportJSON)); !os.IsNotExist(err) {
+		t.Fatalf("invalid review was saved: %v", err)
+	}
+	if _, err := os.Stat(filepath.Join(host.records, "review-failure.json")); err != nil {
+		t.Fatalf("missing failure record: %v", err)
+	}
+}
+
+func TestLegacyInvalidConsistencyReviewIsRefusedBeforePosting(t *testing.T) {
+	t.Setenv("GH_TOKEN", "gh-fixture")
+	// A legacy file bypasses the current writer, as the retained trial does.
+	saved := Saved{Program: Name, PullRequest: SavedPullRequest{Owner: "o", Repo: "r", Number: 7, HeadSHA: "abc"}, Review: invalidConsistencyResult()}
+	b, _ := json.Marshal(saved)
+	path := filepath.Join(t.TempDir(), reportJSON)
+	if err := os.WriteFile(path, b, 0600); err != nil {
+		t.Fatal(err)
+	}
+	called := false
+	end := runPost(context.Background(), newTestHost(t), path, func(string) gitHub { called = true; return &fakeGitHub{} })
+	if end.Status != delegate.StatusFail || called || !strings.Contains(end.Message, "invalid consistency finding") {
+		t.Fatalf("legacy invalid post: %+v, provider constructed=%v", end, called)
+	}
+}
+
+func TestPartialPostingRequiresHonestCoverageAndCannotApprove(t *testing.T) {
+	for _, tc := range []struct {
+		name    string
+		mutate  func(*Saved)
+		refused bool
+	}{
+		{"warned comment", func(*Saved) {}, false},
+		{"missing warning", func(s *Saved) { s.Review.Review.Body = "No findings." }, true},
+		{"approval", func(s *Saved) { s.Review.Review.Event = "APPROVE" }, true},
+		{"contradictory complete", func(s *Saved) { s.Coverage.Complete = true }, true},
+		{"negative count", func(s *Saved) { s.Coverage.FailedSessions = -1 }, true},
+		{"metadata budget mismatch", func(s *Saved) { s.Review.Summary.BudgetExhausted = true }, true},
+		{"legacy budget stop", func(s *Saved) {
+			s.Coverage = nil
+			s.Review.Summary.BudgetExhausted = true
+			s.Review.Review.Event = "APPROVE"
+		}, true},
+		{"legacy degraded comment", func(s *Saved) { s.Coverage = nil; s.Review.Metadata.DegradedDimensions = 1 }, true},
+		{"legacy without known partial facts", func(s *Saved) {
+			s.Coverage = nil
+			s.Review.Review = schemas.GitHubReview{Body: "summary", Event: "APPROVE"}
+		}, false},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			s := Saved{Program: Name, PullRequest: SavedPullRequest{Owner: "o", Repo: "r", Number: 7, HeadSHA: "abc"}, Review: schemas.ReviewResult{Review: schemas.GitHubReview{Body: partialWarning + " One session failed.\n\nNo findings.", Event: "COMMENT"}}, Coverage: &ReportCoverage{FailedSessions: 1, Note: "One session failed."}}
+			tc.mutate(&s)
+			b, _ := json.Marshal(s)
+			path := filepath.Join(t.TempDir(), reportJSON)
+			if err := os.WriteFile(path, b, 0600); err != nil {
+				t.Fatal(err)
+			}
+			_, _, err := readSaved(path)
+			if (err != nil) != tc.refused {
+				t.Fatalf("readSaved error=%v; refused want %v", err, tc.refused)
+			}
+		})
+	}
+}
+
+func TestBudgetPartialAdvisoryReportKeepsFindingsWithoutMergeClaim(t *testing.T) {
+	r, _ := testReviewer(&fakeSessions{}, func(context.Context, orch.Deps) (schemas.ReviewResult, error) {
+		x := sampleResult()
+		x.Summary.BudgetExhausted = true
+		x.Findings = x.Findings[1:]
+		x.Findings[0].Blocking = false
+		x.Review = schemas.GitHubReview{Body: "> **Safe to merge.** One advisory below.", Event: "COMMENT"}
+		return x, nil
+	})
+	host := newTestHost(t)
+	end := runReview(context.Background(), host, ReadBrief("o/r#7", nil), defaults(), io.Discard, r)
+	if end.Status != delegate.StatusPass {
+		t.Fatalf("assembled partial status=%s", end.Status)
+	}
+	saved, _, err := readSaved(host.records)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if strings.Contains(saved.Review.Review.Body, "Safe to merge") || !strings.Contains(saved.Review.Review.Body, "Nil dereference") || !strings.Contains(saved.Review.Review.Body, "The value is used before err is checked.") || strings.Contains(saved.Coverage.Note, "dollar") {
+		t.Fatalf("partial payload lost finding or kept a clean/cost-only claim: %+v", saved)
+	}
+}
+
+func TestPartialReviewKeepsPassButPersistsCoverageAndCannotApprove(t *testing.T) {
+	sessions := &fakeSessions{failed: "session deadline: private provider detail"}
+	r, _ := testReviewer(sessions, func(ctx context.Context, deps orch.Deps) (schemas.ReviewResult, error) {
+		_, _ = deps.App.Harness(ctx, "review", nil, nil, appxOptions())
+		sessions.failed = ""
+		_, _ = deps.App.Harness(ctx, "review", nil, nil, appxOptions())
+		x := sampleResult()
+		x.Findings = nil
+		x.Summary = schemas.ReviewSummary{BySeverity: map[string]int{}}
+		x.Review = schemas.GitHubReview{Body: "> **Safe to merge.** No findings from automated review.", Event: "APPROVE"}
+		return x, nil
+	})
+	host := newTestHost(t)
+	end := runReview(context.Background(), host, ReadBrief("o/r#7", nil), defaults(), io.Discard, r)
+	if end.Status != delegate.StatusPass {
+		t.Fatalf("partial execution status changed: %+v", end)
+	}
+	saved, _, err := readSaved(host.records)
+	if err != nil {
+		t.Fatal(err)
+	}
+	b, _ := json.Marshal(saved)
+	var data map[string]any
+	_ = json.Unmarshal(b, &data)
+	coverage, _ := data["coverage"].(map[string]any)
+	if coverage == nil || coverage["complete"] != false || saved.Review.Review.Event != "COMMENT" || !strings.Contains(saved.Review.Review.Body, "Partial review") || end.Extra["partial"] != true {
+		t.Fatalf("partial report lost its limits: %s; ending=%+v", b, end)
+	}
+	if !strings.HasPrefix(end.Message, "Partial review of ") || coverage["failed_sessions"] != float64(1) {
+		t.Fatalf("partial first line or count missing: %+v; %s", end, b)
+	}
+	if strings.Contains(saved.Review.Review.Body, "Safe to merge") || strings.Contains(saved.Review.Review.Body, "private provider detail") {
+		t.Fatalf("posting payload kept a clean approval or private exception: %s", saved.Review.Review.Body)
+	}
+	t.Setenv("GH_TOKEN", "gh-fixture")
+	gh := &fakeGitHub{}
+	posted := runPost(context.Background(), newTestHost(t), host.records, func(string) gitHub { return gh })
+	if posted.Status != delegate.StatusPass || len(gh.posted) != 1 || gh.posted[0].Event != "COMMENT" || gh.posted[0].Body != saved.Review.Review.Body {
+		t.Fatalf("partial warning changed on posting: %+v; %+v", posted, gh.posted)
+	}
+}