From c96c3e201877553d02869bdddee5abace673a908 Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Tue, 13 Oct 2026 23:17:05 -0400 Subject: [PATCH 1/3] fix: validate consistency findings and retain partial review coverage --- internal/manual/chat/review.md | 13 ++ .../praf/orch/consistency_validity_test.go | 49 +++++++ internal/praf/orch/output.go | 5 + internal/praf/orch/phases.go | 8 +- internal/praf/post.go | 15 +++ internal/praf/report.go | 19 +++ internal/praf/report_validity_test.go | 127 ++++++++++++++++++ internal/praf/review.go | 47 ++++++- internal/praf/schemas/consistency.go | 28 ++++ 9 files changed, 308 insertions(+), 3 deletions(-) create mode 100644 internal/praf/orch/consistency_validity_test.go create mode 100644 internal/praf/report_validity_test.go create mode 100644 internal/praf/schemas/consistency.go diff --git a/internal/manual/chat/review.md b/internal/manual/chat/review.md index f5433a77d..504806d1b 100644 --- a/internal/manual/chat/review.md +++ b/internal/manual/chat/review.md @@ -66,6 +66,19 @@ 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 consistency verdict that claims a 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. + 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 diff --git a/internal/praf/orch/consistency_validity_test.go b/internal/praf/orch/consistency_validity_test.go new file mode 100644 index 000000000..0631b9b7e --- /dev/null +++ b/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) + } + }) + } +} diff --git a/internal/praf/orch/output.go b/internal/praf/orch/output.go index 6e18eef54..e9ea34618 100644 --- a/internal/praf/orch/output.go +++ b/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 { diff --git a/internal/praf/orch/phases.go b/internal/praf/orch/phases.go index ab69b3afc..639a26c96 100644 --- a/internal/praf/orch/phases.go +++ b/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 } diff --git a/internal/praf/post.go b/internal/praf/post.go index fe96aea4c..689cdc65f 100644 --- a/internal/praf/post.go +++ b/internal/praf/post.go @@ -52,6 +52,21 @@ 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 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 } diff --git a/internal/praf/report.go b/internal/praf/report.go index 6016030c8..a58def184 100644 --- a/internal/praf/report.go +++ b/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 + ")" } diff --git a/internal/praf/report_validity_test.go b/internal/praf/report_validity_test.go new file mode 100644 index 000000000..ea0d4b552 --- /dev/null +++ b/internal/praf/report_validity_test.go @@ -0,0 +1,127 @@ +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 complete report", 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 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) + } +} diff --git a/internal/praf/review.go b/internal/praf/review.go index 612f12938..34aaab56c 100644 --- a/internal/praf/review.go +++ b/internal/praf/review.go @@ -229,6 +229,13 @@ func runReview(ctx context.Context, host delegate.Host, request Request, o optio defer cancel() } result, pr, err := r.review(reviewCtx, orch.Deps{App: review, GH: gh, NodeID: Name, Local: tracker, SpentUSD: spent}, in, cfg) + if err == nil { + for _, finding := range result.Findings { + if err = finding.ValidateConsistency(); err != nil { + break + } + } + } cost, sessionCount, callCount := sessions.Spent() if err != nil { _, _ = fmt.Fprintf(notes, "[review] the review ended without a result: %v\n", err) @@ -277,9 +284,46 @@ func runReview(ctx context.Context, host delegate.Host, request Request, o optio run.failNote = strings.TrimSpace(run.failNote + fmt.Sprintf(" %d of its %d agent sessions and single calls failed, so parts of the review may be missing; the first with: %s", calls.Failed, calls.Total, firstSentence(calls.FirstErr))) } + coverage := ReportCoverage{ + FailedSessions: calls.SessionFailed, FailedCalls: calls.Failed - calls.SessionFailed, + DegradedDimensions: result.Metadata.DegradedDimensions, BudgetExhausted: result.Summary.BudgetExhausted, + StoppedAt: run.cutAt, + } + coverage.Complete = calls.Failed == 0 && coverage.DegradedDimensions == 0 && !coverage.BudgetExhausted && coverage.StoppedAt == "" + run.partial = !coverage.Complete + if run.partial { + // The posting payload names missing work, not raw local/provider + // exceptions that may contain private paths or account details. + var limits []string + if calls.Failed > 0 { + limits = append(limits, fmt.Sprintf("%d agent sessions and %d single calls failed; parts of the review may be missing.", coverage.FailedSessions, coverage.FailedCalls)) + } + if run.cutAt != "" { + limits = append(limits, cutSentence(run.cutAt, run.wall)) + } + if coverage.BudgetExhausted { + limits = append(limits, "The dollar limit stopped new work; later checks were skipped.") + } + if coverage.DegradedDimensions > 0 { + limits = append(limits, fmt.Sprintf("%d review dimensions degraded; parts of the change may be missing.", coverage.DegradedDimensions)) + } + coverage.Note = strings.Join(limits, " ") + if run.failNote == "" && run.cutAt == "" { + run.failNote = coverage.Note + } + warning := partialWarning + " " + coverage.Note + if result.Review.Event == "APPROVE" { + result.Review.Event = "COMMENT" + // The clean-approval body can say "Safe to merge" even when no + // findings means missing reviewers. Do not preserve that claim. + result.Review.Body = warning + "\n\nThis report covers only the work that completed." + } else { + result.Review.Body = warning + "\n\n" + result.Review.Body + } + } host.Stage(delegate.StageRecord{Stage: stageReport, Status: "running", Data: stageData(map[string]any{"doing": "writing the review"})}) - saved := Saved{Program: Name, Focus: request.Focus, Review: result, + saved := Saved{Program: Name, Focus: request.Focus, Review: result, Coverage: &coverage, PullRequest: SavedPullRequest{Owner: t.Owner, Repo: t.Repo, Number: t.Number, URL: t.URL()}} if pr != nil { saved.PullRequest.Title, saved.PullRequest.HeadSHA = pr.Title, pr.HeadSHA @@ -302,6 +346,7 @@ func runReview(ctx context.Context, host delegate.Host, request Request, o optio CostUSD: cost, Deliverable: account(result, run, files), Extra: map[string]any{ + "partial": run.partial, "pull_request": t.String(), "reports": files, "findings": result.Summary.TotalFindings, "blocking": result.Summary.BlockingCount, "sessions": sessionCount, "calls": callCount, "cut_at": run.cutAt, }, diff --git a/internal/praf/schemas/consistency.go b/internal/praf/schemas/consistency.go new file mode 100644 index 000000000..c404792f9 --- /dev/null +++ b/internal/praf/schemas/consistency.go @@ -0,0 +1,28 @@ +package schemas + +import ( + "fmt" + "strings" +) + +// A negative consistency verdict claims a verified disagreement at real code +// locations. Its optional schema fields are defaults for holds=true, not a +// license to publish a title without the supporting finding. +func validateConsistency(dimension, title, body, path string, start, end int) error { + if !strings.EqualFold(strings.TrimSpace(dimension), "consistency-verify") { + return nil + } + if strings.TrimSpace(title) == "" || strings.TrimSpace(body) == "" || + strings.TrimSpace(path) == "" || start <= 0 || (end != 0 && end < start) { + return fmt.Errorf("invalid consistency finding: a defect needs a title, explanation and code location with a valid line range") + } + return nil +} + +func (f ReviewFinding) ValidateConsistency() error { + return validateConsistency(f.DimensionID, f.Title, f.Body, f.FilePath, f.LineStart, f.LineEnd) +} + +func (f ScoredFinding) ValidateConsistency() error { + return validateConsistency(f.DimensionID, f.Title, f.Body, f.FilePath, f.LineStart, f.LineEnd) +} From d2e420cd5e45670a0c208c26c7bdf206a97c4dc3 Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Tue, 13 Oct 2026 23:21:45 -0400 Subject: [PATCH 2/3] fix: keep partial posting payloads honest across legacy and advisory reports --- internal/manual/chat/review.md | 5 +++-- internal/praf/orch/output.go | 20 ++++++++++++++++- internal/praf/orch/output_test.go | 13 +++++++++++ internal/praf/post.go | 3 +++ internal/praf/report_validity_test.go | 31 ++++++++++++++++++++++++++- internal/praf/review.go | 15 ++++++------- 6 files changed, 74 insertions(+), 13 deletions(-) diff --git a/internal/manual/chat/review.md b/internal/manual/chat/review.md index 504806d1b..472473b81 100644 --- a/internal/manual/chat/review.md +++ b/internal/manual/chat/review.md @@ -74,10 +74,11 @@ posted as an approval; a valid partial report can still be posted as a comment o request for changes on your yes. Terminal success means the report was assembled, not that every check completed. -A consistency verdict that claims a defect must include its explanation and code +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. +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. diff --git a/internal/praf/orch/output.go b/internal/praf/orch/output.go index e9ea34618..ea9f414d4 100644 --- a/internal/praf/orch/output.go +++ b/internal/praf/orch/output.go @@ -339,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 { @@ -347,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 } diff --git a/internal/praf/orch/output_test.go b/internal/praf/orch/output_test.go index c8e7fd8ec..e08108b0c 100644 --- a/internal/praf/orch/output_test.go +++ b/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 } diff --git a/internal/praf/post.go b/internal/praf/post.go index 689cdc65f..18e689ffe 100644 --- a/internal/praf/post.go +++ b/internal/praf/post.go @@ -57,6 +57,9 @@ func readSaved(path string) (Saved, string, error) { 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 || diff --git a/internal/praf/report_validity_test.go b/internal/praf/report_validity_test.go index ea0d4b552..38f2ba926 100644 --- a/internal/praf/report_validity_test.go +++ b/internal/praf/report_validity_test.go @@ -63,7 +63,13 @@ func TestPartialPostingRequiresHonestCoverageAndCannotApprove(t *testing.T) { {"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 complete report", func(s *Saved) { + {"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}, @@ -84,6 +90,29 @@ func TestPartialPostingRequiresHonestCoverageAndCannotApprove(t *testing.T) { } } +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) { diff --git a/internal/praf/review.go b/internal/praf/review.go index 34aaab56c..480c2060b 100644 --- a/internal/praf/review.go +++ b/internal/praf/review.go @@ -302,7 +302,7 @@ func runReview(ctx context.Context, host delegate.Host, request Request, o optio limits = append(limits, cutSentence(run.cutAt, run.wall)) } if coverage.BudgetExhausted { - limits = append(limits, "The dollar limit stopped new work; later checks were skipped.") + limits = append(limits, "A budget limit stopped new work; later checks were skipped.") } if coverage.DegradedDimensions > 0 { limits = append(limits, fmt.Sprintf("%d review dimensions degraded; parts of the change may be missing.", coverage.DegradedDimensions)) @@ -311,15 +311,12 @@ func runReview(ctx context.Context, host delegate.Host, request Request, o optio if run.failNote == "" && run.cutAt == "" { run.failNote = coverage.Note } - warning := partialWarning + " " + coverage.Note - if result.Review.Event == "APPROVE" { - result.Review.Event = "COMMENT" - // The clean-approval body can say "Safe to merge" even when no - // findings means missing reviewers. Do not preserve that claim. - result.Review.Body = warning + "\n\nThis report covers only the work that completed." - } else { - result.Review.Body = warning + "\n\n" + result.Review.Body + var body strings.Builder + body.WriteString(partialWarning + " " + coverage.Note + "\n\nThis report covers only the work that completed.\n") + for i, finding := range sortedFindings(result.Findings) { + body.WriteString(markdownFinding(i+1, finding, run.checkout)) } + result.Review = orch.PartialReviewPayload(result.Review, body.String()) } host.Stage(delegate.StageRecord{Stage: stageReport, Status: "running", Data: stageData(map[string]any{"doing": "writing the review"})}) From 9f0d5d7f31b282996a6f7bdb3926a89d9a42f5b1 Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Tue, 13 Oct 2026 23:34:14 -0400 Subject: [PATCH 3/3] docs: record review validity and partial coverage for #1820 --- .../1820-review-validity-partial-coverage.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 docs/changes/unreleased/1820-review-validity-partial-coverage.md diff --git a/docs/changes/unreleased/1820-review-validity-partial-coverage.md b/docs/changes/unreleased/1820-review-validity-partial-coverage.md new file mode 100644 index 000000000..2c30ee9b9 --- /dev/null +++ b/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.