From 968c5a72e6dc5173a5f7b54ade05d0b5cd6102e5 Mon Sep 17 00:00:00 2001 From: piekstra Date: Tue, 3 Nov 2026 06:43:14 -0500 Subject: [PATCH 1/2] fix(review): post findings when the rollup conflicts with the review policy The rollup prompt did not state the review policy, so the model proposed request_changes for a major finding under major_event: comment and the decoder rejected it. A response that also omitted schema_version reported only the version on the first attempt, so the retry failed on the event with no attempts left and the run posted nothing. The prompt now states the policy. The decoder accepts an absent schema_version and no longer judges the event; reviewplan decides the posted event from severities and policy, and a major finding never approves. Rollup output that stays invalid after the retry falls back to posting every finding without deduplication. Twapp-Session: 59d912f3-982f-402e-8bd8-3e31269a28b0 --- README.md | 2 +- docs/llm-task-artifacts.md | 20 ++- internal/llm/contracts.go | 45 +----- internal/llm/contracts_test.go | 46 +++++- internal/pipeline/pipeline.go | 26 ++- internal/pipeline/pipeline_test.go | 148 +++++++++++++++++- internal/pipeline/prompts.go | 13 +- internal/pipeline/prompts_test.go | 42 ++++- .../pipeline/sensitive_value_rule_test.go | 2 +- .../pipeline/thread_analysis_prompt_test.go | 14 +- internal/review/review.go | 5 + internal/reviewplan/effective_event_test.go | 74 +++++---- internal/reviewplan/reviewplan.go | 41 +++-- internal/reviewplan/summary_bounded_test.go | 1 + 14 files changed, 352 insertions(+), 127 deletions(-) diff --git a/README.md b/README.md index 55150b6a..e8db36ae 100644 --- a/README.md +++ b/README.md @@ -681,7 +681,7 @@ Supported values: | `llm.model_map` keys | `small`, `medium`, `large` | | `llm_runtimes..max_effort` keys | `small`, `medium`, `large`; values `low`, `medium`, `high`, plus `xhigh` and `max` for `claude_cli`, `codex_cli`, `openai_api`, and `pi_rpc`. `anthropic_api` supports through `high`. Effective extended-effort support depends on the selected model and, for CLI adapters, the installed CLI. | | `llm.reviewer_model_tier` | `small`, `medium`, `large` | -| `review_policy.major_event` | `comment`, `request_changes` | +| `review_policy.major_event` | `comment`, `request_changes`. Sets the review event when the highest severity is major. Blocking findings always request changes, a major finding never approves, and the rollup model's proposed event cannot override either rule. Under `request_changes`, the rollup model may soften a major-only review to a comment. | | `review_policy.resolve_threads` | `auto`, `never` | | `data.retention.enforcement` | `at_write` applies review-time pruning before each `cr review`; `manual_only` disables review-time pruning and leaves `cr data prune` as the explicit maintenance path. Review-time pruning runs only when no other cr instance is mid-run (it requires the data-root active-runs lock exclusively) and is skipped, not queued, on contention. | | `data.keep_workbench` | `true` retains the run checkout under `workbench/` after a successful run. Failed and errored runs always retain it. | diff --git a/docs/llm-task-artifacts.md b/docs/llm-task-artifacts.md index 409e783a..e45f0c2c 100644 --- a/docs/llm-task-artifacts.md +++ b/docs/llm-task-artifacts.md @@ -155,12 +155,20 @@ their retry semantics, and new isolated thread tasks use the existing status contract. `failed_blocking` means the task prevents dependent phases from safely running. -Selection and rollup failures are blocking. `reviewrun` is the sole owner of -live planner-error outcomes: blocking LLM failures leave fresh and resumed runs -`incomplete`; transient failures leave resumed runs `incomplete` but fail fresh -runs; terminal failures fail either kind; cancellation leaves the current -outcome untouched. The normal resume gate can therefore rerun only the failed -task and downstream work after an interruption or durable blocking failure. +Selection failures and rollup provider failures are blocking. `reviewrun` is the +sole owner of live planner-error outcomes: blocking LLM failures leave fresh and +resumed runs `incomplete`; transient failures leave resumed runs `incomplete` but +fail fresh runs; terminal failures fail either kind; cancellation leaves the +current outcome untouched. The normal resume gate can therefore rerun only the +failed task and downstream work after an interruption or durable blocking +failure. + +Rollup output that stays invalid after the correction retry does not stop the +run, because reviewer findings must reach the pull request. The rollup task keeps +`failed_blocking` status with both attempts recorded, and the plan uses a fallback +rollup: every finding, most severe first, with no deduplication and a note in the +posted review body. The event comes from finding severities and +`review_policy.major_event`. Provider start/wait failures may have empty `attempts` because no structured output existed. When a provider session ID is known, retry should seed the next diff --git a/internal/llm/contracts.go b/internal/llm/contracts.go index 13e4a967..23a8a317 100644 --- a/internal/llm/contracts.go +++ b/internal/llm/contracts.go @@ -132,8 +132,7 @@ type FindingsOptions struct { // RollupOptions contains context needed to validate rollup output. type RollupOptions struct { - FindingSeverities map[review.FindingID]review.Severity - MajorEventRequestsChanges bool + FindingSeverities map[review.FindingID]review.Severity } type selectionWire struct { @@ -184,7 +183,9 @@ type anchorWire struct { } type rollupWire struct { - SchemaVersion int `json:"schema_version"` + // SchemaVersion is a pointer because models omit it from otherwise valid + // rollups, and an absent version does not name a different schema. + SchemaVersion *int `json:"schema_version"` ReviewEvent string `json:"review_event"` ReviewEventRationale string `json:"review_event_rationale"` DedupeLog []dedupeEntryWire `json:"dedupe_log"` @@ -571,8 +572,8 @@ func DecodeRollup(data []byte, opts RollupOptions) (review.Rollup, error) { if err := decodeStrict(data, &wire); err != nil { return review.Rollup{}, err } - if wire.SchemaVersion != SchemaVersion { - return review.Rollup{}, fmt.Errorf("llm: unsupported rollup schema_version %d", wire.SchemaVersion) + if wire.SchemaVersion != nil && *wire.SchemaVersion != SchemaVersion { + return review.Rollup{}, fmt.Errorf("llm: unsupported rollup schema_version %d", *wire.SchemaVersion) } event, err := review.ParseReviewEvent(wire.ReviewEvent) if err != nil { @@ -589,9 +590,6 @@ func DecodeRollup(data []byte, opts RollupOptions) (review.Rollup, error) { if err != nil { return review.Rollup{}, err } - if err := validateReviewEvent(event, ordered, opts); err != nil { - return review.Rollup{}, err - } return review.Rollup{ ReviewEvent: event, ReviewEventRationale: sanitize(wire.ReviewEventRationale), @@ -705,37 +703,6 @@ func decodeOrderedFindings(raw []string, known map[review.FindingID]review.Sever return ordered, nil } -func validateReviewEvent(event review.ReviewEvent, ordered []review.FindingID, opts RollupOptions) error { - var blocking, major int - for _, id := range ordered { - switch opts.FindingSeverities[id] { - case review.SeverityBlocking: - blocking++ - case review.SeverityMajor: - major++ - case review.SeverityMinor, review.SeverityNits: - default: - return fmt.Errorf("llm: invalid severity for ordered finding %q", id) - } - } - switch { - case blocking > 0: - if event != review.ReviewEventRequestChanges { - return fmt.Errorf("llm: blocking findings require request_changes") - } - case major > 0: - if event == review.ReviewEventApprove { - return fmt.Errorf("llm: major findings cannot approve") - } - if event == review.ReviewEventRequestChanges && !opts.MajorEventRequestsChanges { - return fmt.Errorf("llm: major findings cannot request_changes under current policy") - } - case event == review.ReviewEventRequestChanges: - return fmt.Errorf("llm: request_changes requires blocking findings or major policy") - } - return nil -} - func knownID(known map[review.FindingID]review.Severity, id review.FindingID) bool { if !id.Assigned() { return false diff --git a/internal/llm/contracts_test.go b/internal/llm/contracts_test.go index a0af2ca9..e7cbbe3e 100644 --- a/internal/llm/contracts_test.go +++ b/internal/llm/contracts_test.go @@ -436,11 +436,47 @@ func TestDecodeRollup(t *testing.T) { assertRollupError(t, known, `{"schema_version":1,"review_event":"comment","review_event_rationale":"x","ordered_findings":["missing","f-1","f-2","f-3"]}`, "unknown ordered finding ID") assertRollupError(t, known, `{"schema_version":1,"review_event":"comment","review_event_rationale":"x","ordered_findings":["f-1","f-1","f-2","f-3"]}`, "appears more than once") assertRollupError(t, known, `{"schema_version":1,"review_event":"comment","review_event_rationale":"x","ordered_findings":["f-1","f-2"]}`, "does not cover") - assertRollupError(t, known, `{"schema_version":1,"review_event":"approve","review_event_rationale":"x","ordered_findings":["f-1","f-2","f-3"]}`, "major findings cannot approve") - assertRollupError(t, map[review.FindingID]review.Severity{"f-1": review.SeverityBlocking}, `{"schema_version":1,"review_event":"comment","review_event_rationale":"x","ordered_findings":["f-1"]}`, "blocking") - assertRollupError(t, map[review.FindingID]review.Severity{"f-1": review.SeverityMinor}, `{"schema_version":1,"review_event":"request_changes","review_event_rationale":"x","ordered_findings":["f-1"]}`, "request_changes") - if _, err := DecodeRollup([]byte(`{"schema_version":1,"review_event":"request_changes","review_event_rationale":"x","ordered_findings":["f-1"]}`), RollupOptions{FindingSeverities: map[review.FindingID]review.Severity{"f-1": review.SeverityMajor}, MajorEventRequestsChanges: true}); err != nil { - t.Fatalf("DecodeRollup major request_changes with policy: %v", err) +} + +// The model's event is a proposal that reviewplan resolves against the review +// policy, so a proposal that conflicts with the policy must not fail decoding. +func TestDecodeRollupAcceptsEventsThatConflictWithPolicy(t *testing.T) { + for _, tc := range []struct { + name string + severity review.Severity + event string + want review.ReviewEvent + }{ + {"major request_changes", review.SeverityMajor, "request_changes", review.ReviewEventRequestChanges}, + {"major approve", review.SeverityMajor, "approve", review.ReviewEventApprove}, + {"blocking comment", review.SeverityBlocking, "comment", review.ReviewEventComment}, + {"minor request_changes", review.SeverityMinor, "request_changes", review.ReviewEventRequestChanges}, + } { + t.Run(tc.name, func(t *testing.T) { + got, err := DecodeRollup([]byte(`{"schema_version":1,"review_event":"`+tc.event+`","review_event_rationale":"x","ordered_findings":["f-1"]}`), RollupOptions{ + FindingSeverities: map[review.FindingID]review.Severity{"f-1": tc.severity}, + }) + if err != nil { + t.Fatalf("DecodeRollup: %v", err) + } + if got.ReviewEvent != tc.want { + t.Fatalf("ReviewEvent = %q, want %q", got.ReviewEvent, tc.want) + } + }) + } +} + +func TestDecodeRollupSchemaVersion(t *testing.T) { + known := map[review.FindingID]review.Severity{"f-1": review.SeverityMajor} + const fields = `"review_event":"comment","review_event_rationale":"x","ordered_findings":["f-1"]` + + if _, err := DecodeRollup([]byte(`{`+fields+`}`), RollupOptions{FindingSeverities: known}); err != nil { + t.Fatalf("DecodeRollup with absent schema_version: %v", err) + } + for _, version := range []string{"0", "2"} { + t.Run("rejects "+version, func(t *testing.T) { + assertRollupError(t, known, `{"schema_version":`+version+`,`+fields+`}`, "unsupported rollup schema_version "+version) + }) } } diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 74163bdc..311596fd 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -955,7 +955,7 @@ func executeLLMPhases(ctx context.Context, opts Options, req Request, mode execu return nil, false, Failure(FailureTerminal, err) } rollupModel, rollupEffort := rollupRuntimeConfig.model, rollupRuntimeConfig.effort - rollupPrompt, err := buildRollupPrompt(prepared.reviewPR, reviewerRun.findings, reviewerRun.failures, reviewerCoverage, threadFailureSummaries(threadFailures)) + rollupPrompt, err := buildRollupPrompt(prepared.reviewPR, reviewerRun.findings, reviewerRun.failures, reviewerCoverage, req.MajorRequestChanges, threadFailureSummaries(threadFailures)) if err != nil { return nil, false, Failure(FailureTerminal, err) } @@ -986,15 +986,18 @@ func executeLLMPhases(ctx context.Context, opts Options, req Request, mode execu resumeSessionID: namedSession.resumeID(), }, func(data []byte) (review.Rollup, error) { return llm.DecodeRollup(data, llm.RollupOptions{ - FindingSeverities: findingSeverities(reviewerRun.findings), - MajorEventRequestsChanges: req.MajorRequestChanges, + FindingSeverities: findingSeverities(reviewerRun.findings), }) }) if checkpointErr := namedSession.checkpointSessionID(ctx, rollupSession, opts.now()); checkpointErr != nil { return nil, false, checkpointErr } if err != nil { - return executionPhaseFailure(err) + if !errors.Is(err, ErrStructuredOutputInvalidAfterRetry) { + return executionPhaseFailure(err) + } + opts.emitWarning("rollup output stayed invalid after retry; posting every finding without deduplication") + rollup = fallbackRollup(reviewerRun.findings) } result.Rollup = rollup result.Sessions = appendSessionIfPresent(result.Sessions, rollupLedgerSession) @@ -1027,6 +1030,21 @@ func executeLLMPhases(ctx context.Context, opts Options, req Request, mode execu return reviewerRun.findingSessions, false, nil } +// fallbackRollup keeps reviewer findings postable when the rollup model cannot +// return valid output. Only deduplication is lost: the event never depended on +// the model's choice beyond softening a major-only request_changes. +func fallbackRollup(findings []review.Finding) review.Rollup { + ordered := make([]review.FindingID, 0, len(findings)) + for _, severity := range review.SeverityOrder() { + for _, finding := range findings { + if finding.Severity == severity { + ordered = append(ordered, finding.ID) + } + } + } + return review.Rollup{Fallback: true, OrderedFindings: ordered} +} + func executionPhaseFailure(err error) (map[review.FindingID]string, bool, error) { return nil, errors.Is(err, errLLMTaskFailedBlocking), err } diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 58f7dacd..40447733 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -697,8 +697,7 @@ func TestReviewPipelineAcceptanceHarnessResumesFailedDurableTask(t *testing.T) { reviewer := fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4) reviewer.Response.ReviewerToolEvidence = &llm.ReviewerToolEvidence{DiffStatus: llm.DiffToolStatusIncomplete} firstAdapter.Queue(reviewer) - firstAdapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"missing-finding"}), 30, 6)) - firstAdapter.Queue(fakeLLMResult("rollup-retry-session", rollupJSON("comment", []string{"missing-finding"}), 30, 6)) + firstAdapter.Queue(llm.FakeResult{SessionID: "rollup-retry-session", WaitErr: errors.New("rollup provider failed")}) _, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: firstAdapter, @@ -711,8 +710,8 @@ func TestReviewPipelineAcceptanceHarnessResumesFailedDurableTask(t *testing.T) { NewActionID: actionSequence(), MaxConcurrency: 1, }, req) - if err == nil || !errors.Is(err, ErrStructuredOutputInvalidAfterRetry) { - t.Fatalf("first DryRun error = %v, want invalid rollup after retry", err) + if err == nil || !errors.Is(err, errLLMTaskFailedBlocking) { + t.Fatalf("first DryRun error = %v, want blocking rollup failure", err) } stored, err := store.GetRun(ctx, run.RunID) if err != nil { @@ -3352,8 +3351,7 @@ func TestLiveResumeCompletedSelectionAndReviewersRerunsOnlyRollup(t *testing.T) firstAdapter := &llm.FakeAdapter{NameValue: "fake-llm", SupportsResumeValue: true} firstAdapter.Queue(fakeLLMResult("selection-session", selectionJSON("harness:reviewer", "main.go"), 10, 2)) firstAdapter.Queue(fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) - firstAdapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"missing-finding"}), 30, 6)) - firstAdapter.Queue(fakeLLMResult("rollup-retry-session", rollupJSON("comment", []string{"missing-finding"}), 30, 6)) + firstAdapter.Queue(llm.FakeResult{SessionID: "rollup-retry-session", WaitErr: errors.New("rollup provider failed")}) findingID := func() (review.FindingID, error) { return review.FindingID("finding-1"), nil } _, err := liveForTest(ctx, Options{ @@ -3368,8 +3366,8 @@ func TestLiveResumeCompletedSelectionAndReviewersRerunsOnlyRollup(t *testing.T) NewActionID: actionSequence(), MaxConcurrency: 1, }, req, run) - if err == nil || !errors.Is(err, ErrStructuredOutputInvalidAfterRetry) { - t.Fatalf("first Live error = %v, want invalid rollup after retry", err) + if err == nil || !errors.Is(err, errLLMTaskFailedBlocking) { + t.Fatalf("first Live error = %v, want blocking rollup failure", err) } stored, err := store.GetRun(ctx, run.RunID) if err != nil { @@ -3426,6 +3424,140 @@ func TestLiveResumeCompletedSelectionAndReviewersRerunsOnlyRollup(t *testing.T) } } +// A rollup that omits schema_version and proposes request_changes for a major +// finding under major_event: comment failed the whole run before this test +// existed: the first attempt reported only the missing version, and the retry, +// the last attempt, then hit the event conflict. Nothing was posted. +func TestLiveMajorFindingUnderCommentPolicyPostsCommentForConflictingRollup(t *testing.T) { + ctx := context.Background() + store := openPipelineStore(t) + defer closeStore(t, store) + provider, req := dryRunHarness(t) + req.MajorRequestChanges = false + run := allocateLiveRun(t, store, provider, req, "run-major-comment-policy") + adapter := &llm.FakeAdapter{NameValue: "fake-llm", SupportsResumeValue: true} + adapter.Queue(fakeLLMResult("selection-session", selectionJSON("harness:reviewer", "main.go"), 10, 2)) + adapter.Queue(fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) + adapter.Queue(fakeLLMResult("rollup-session", `{"review_event":"request_changes","review_event_rationale":"major finding","dedupe_log":[],"ordered_findings":["finding-1"]}`, 30, 6)) + + result, err := liveForTest(ctx, Options{ + Provider: provider, + Adapter: adapter, + Store: store, + NamedSessions: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), + Now: fixedNow, + NewSessionRowID: sequence("session"), + NewFindingID: func() (review.FindingID, error) { return review.FindingID("finding-1"), nil }, + NewActionID: actionSequence(), + MaxConcurrency: 1, + }, req, run) + if err != nil { + t.Fatalf("Live: %v", err) + } + if calls := len(adapter.Requests()) + len(adapter.Resumes()); calls != 3 { + t.Fatalf("adapter calls = %d, want selection, reviewer and one rollup attempt", calls) + } + assertSubmittedReview(t, result, review.ReviewEventComment, "Fix this") + if strings.Contains(result.Plan.RollupMarkdown, rollupFallbackNoteMarker) { + t.Fatalf("valid rollup rendered the fallback note:\n%s", result.Plan.RollupMarkdown) + } +} + +// Rollup output that stays invalid after the retry must not cost the run its +// findings, and it must never turn a blocking or major finding into an approval. +func TestLiveInvalidRollupAfterRetryStillPostsEveryFinding(t *testing.T) { + for _, tc := range []struct { + name string + severity string + requestChanges bool + want review.ReviewEvent + }{ + {"major under comment policy", "major", false, review.ReviewEventComment}, + {"major under request_changes policy", "major", true, review.ReviewEventRequestChanges}, + {"blocking", "blocking", false, review.ReviewEventRequestChanges}, + {"minor", "minor", false, review.ReviewEventApprove}, + } { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + store := openPipelineStore(t) + defer closeStore(t, store) + provider, req := dryRunHarness(t) + req.MajorRequestChanges = tc.requestChanges + run := allocateLiveRun(t, store, provider, req, "run-invalid-rollup") + adapter := &llm.FakeAdapter{NameValue: "fake-llm", SupportsResumeValue: true} + adapter.Queue(fakeLLMResult("selection-session", selectionJSON("harness:reviewer", "main.go"), 10, 2)) + adapter.Queue(fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", tc.severity, 2, "Fix this"), 20, 4)) + adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"missing-finding"}), 30, 6)) + adapter.Queue(fakeLLMResult("rollup-retry-session", rollupJSON("comment", []string{"missing-finding"}), 30, 6)) + var warnings bytes.Buffer + + result, err := liveForTest(ctx, Options{ + Provider: provider, + Adapter: adapter, + Store: store, + NamedSessions: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), + Now: fixedNow, + NewSessionRowID: sequence("session"), + NewFindingID: func() (review.FindingID, error) { return review.FindingID("finding-1"), nil }, + NewActionID: actionSequence(), + MaxConcurrency: 1, + Warnings: &warnings, + }, req, run) + if err != nil { + t.Fatalf("Live: %v", err) + } + assertSubmittedReview(t, result, tc.want, "Fix this") + if !result.Rollup.Fallback || !strings.Contains(result.Plan.RollupMarkdown, rollupFallbackNoteMarker) { + t.Fatalf("rollup = %#v, body:\n%s\nwant the fallback rollup and its note", result.Rollup, result.Plan.RollupMarkdown) + } + if !strings.Contains(warnings.String(), "rollup output stayed invalid") { + t.Fatalf("warnings = %q, want the fallback warning", warnings.String()) + } + meta, ok, err := llmlifecycle.ReadMetadata(lifecyclePaths(ArtifactPathsFromDir(run.ArtifactPath)), orchestratorRollupStage) + if err != nil || !ok || meta.Status != llmTaskStatusFailedBlocking || len(meta.Attempts) != 2 { + t.Fatalf("rollup metadata = %#v ok %v err %v, want the failed attempts kept for diagnosis", meta, ok, err) + } + }) + } +} + +func TestFallbackRollupKeepsEveryFindingMostSevereFirst(t *testing.T) { + got := fallbackRollup([]review.Finding{ + {ID: "nit", Severity: review.SeverityNits}, + {ID: "major-1", Severity: review.SeverityMajor}, + {ID: "blocking", Severity: review.SeverityBlocking}, + {ID: "minor", Severity: review.SeverityMinor}, + {ID: "major-2", Severity: review.SeverityMajor}, + }) + want := []review.FindingID{"blocking", "major-1", "major-2", "minor", "nit"} + if !reflect.DeepEqual(got.OrderedFindings, want) || !got.Fallback || got.ReviewEvent != "" || len(got.DedupeLog) != 0 { + t.Fatalf("fallbackRollup = %#v, want ordered %v with no proposed event and no dedupe", got, want) + } +} + +const rollupFallbackNoteMarker = "duplicate findings were not merged" + +func assertSubmittedReview(t *testing.T, result Result, wantEvent review.ReviewEvent, wantBody string) { + t.Helper() + var submit *reviewplan.SubmitReviewPayload + for _, action := range result.Plan.Actions { + if action.Kind == reviewplan.ActionKindSubmitReview { + submit = action.SubmitReview + } + } + if submit == nil { + t.Fatalf("plan actions = %#v, want a submit-review action", result.Plan.Actions) + } + if submit.Event != wantEvent { + t.Fatalf("submitted event = %q, want %q", submit.Event, wantEvent) + } + if !strings.Contains(submit.Body, wantBody) { + t.Fatalf("submitted body omits finding %q:\n%s", wantBody, submit.Body) + } +} + func TestRunStructuredTaskRejectsAdapterMismatchBeforeRetry(t *testing.T) { ctx := context.Background() artifacts := ArtifactPathsFromDir(t.TempDir()) diff --git a/internal/pipeline/prompts.go b/internal/pipeline/prompts.go index 41fef957..0fed2b78 100644 --- a/internal/pipeline/prompts.go +++ b/internal/pipeline/prompts.go @@ -822,10 +822,10 @@ func selectionThreadStatus(thread threadcontext.Thread) string { } } -func buildRollupPrompt(pr gitprovider.PR, findings []review.Finding, reviewerFailures []ReviewerFailure, reviewerCoverage []reviewplan.ReviewerCoverageSummary, threadFailures ...[]reviewplan.ThreadAnalysisFailureSummary) (string, error) { +func buildRollupPrompt(pr gitprovider.PR, findings []review.Finding, reviewerFailures []ReviewerFailure, reviewerCoverage []reviewplan.ReviewerCoverageSummary, majorRequestsChanges bool, threadFailures ...[]reviewplan.ThreadAnalysisFailureSummary) (string, error) { payload := map[string]any{ "task": "dedupe findings and return rollup JSON only", - "output_contract": rollupOutputContract(findings), + "output_contract": rollupOutputContract(findings, majorRequestsChanges), "schema": "rollup", "pr": promptPRFromPR(pr), "findings": rollupFindingsPrompt(findings), @@ -1128,11 +1128,15 @@ func findingsOutputContractWithRelocations(agentID string, changedFiles []string return contract } -func rollupOutputContract(findings []review.Finding) outputContract { +func rollupOutputContract(findings []review.Finding, majorRequestsChanges bool) outputContract { findingIDs := make([]string, 0, len(findings)) for _, finding := range findings { findingIDs = append(findingIDs, finding.ID.String()) } + majorEvent := "review_event must be comment when the highest kept severity is major, because this profile's review_policy.major_event is comment." + if majorRequestsChanges { + majorEvent = "review_event should be request_changes when the highest kept severity is major, because this profile's review_policy.major_event is request_changes; comment is also accepted." + } return outputContract{ Instructions: []string{ "Return exactly one raw JSON object. Do not wrap it in Markdown fences.", @@ -1143,6 +1147,9 @@ func rollupOutputContract(findings []review.Finding) outputContract { "dedupe_log kept and dropped values must contain finding ID strings only, never finding objects.", "Use finding location only to distinguish findings during dedupe; do not include finding fields such as severity, file_path, location, body, anchor, or finding_id in the response.", "dedupe_log must be an empty array when no findings are duplicates.", + "review_event must be request_changes when any kept finding is blocking.", + majorEvent, + "review_event must be approve or comment, never request_changes, when every kept finding is minor or nits or no findings remain.", llm.SensitiveValueRule, }, ResponseSchema: map[string]any{ diff --git a/internal/pipeline/prompts_test.go b/internal/pipeline/prompts_test.go index d4800ed8..68595547 100644 --- a/internal/pipeline/prompts_test.go +++ b/internal/pipeline/prompts_test.go @@ -541,7 +541,7 @@ func TestRollupPromptPreservesLocationForDedupeWithoutRawAnchors(t *testing.T) { Status: reviewerCoverageCompleteBroad, Scope: []string{"main.go"}, InspectedFiles: []string{"main.go"}, - }}) + }}, false) if err != nil { t.Fatalf("buildRollupPrompt: %v", err) } @@ -577,6 +577,38 @@ func TestRollupPromptPreservesLocationForDedupeWithoutRawAnchors(t *testing.T) { } } +// The model picks request_changes for a major finding unless the prompt states +// the profile's policy, and the prompt fingerprint must separate the two. +func TestRollupPromptStatesMajorEventPolicy(t *testing.T) { + findings := []review.Finding{{ID: "finding-1", Severity: review.SeverityMajor, FilePath: "main.go"}} + comment, err := buildRollupPrompt(gitprovider.PR{}, findings, nil, nil, false) + if err != nil { + t.Fatalf("buildRollupPrompt comment policy: %v", err) + } + requestChanges, err := buildRollupPrompt(gitprovider.PR{}, findings, nil, nil, true) + if err != nil { + t.Fatalf("buildRollupPrompt request_changes policy: %v", err) + } + if !strings.Contains(comment, "review_event must be comment when the highest kept severity is major") || + !strings.Contains(comment, "review_policy.major_event is comment") { + t.Fatalf("comment policy prompt does not state the policy:\n%s", comment) + } + if !strings.Contains(requestChanges, "review_event should be request_changes when the highest kept severity is major") || + !strings.Contains(requestChanges, "review_policy.major_event is request_changes") { + t.Fatalf("request_changes policy prompt does not state the policy:\n%s", requestChanges) + } + for name, prompt := range map[string]string{"comment": comment, "request_changes": requestChanges} { + for _, rule := range []string{ + "review_event must be request_changes when any kept finding is blocking", + "never request_changes, when every kept finding is minor or nits", + } { + if !strings.Contains(prompt, rule) { + t.Fatalf("%s policy prompt omits %q:\n%s", name, rule, prompt) + } + } + } +} + func TestRollupPromptAndFingerprintChangeWhenReviewerCoverageChanges(t *testing.T) { findings := largeRollupFindings(1, "main.go", "body") baseCoverage := []reviewplan.ReviewerCoverageSummary{{ @@ -592,11 +624,11 @@ func TestRollupPromptAndFingerprintChangeWhenReviewerCoverageChanges(t *testing. InspectedFiles: []string{"main.go"}, SkippedFiles: []string{"main.go"}, }} - basePrompt, err := buildRollupPrompt(gitprovider.PR{}, findings, nil, baseCoverage) + basePrompt, err := buildRollupPrompt(gitprovider.PR{}, findings, nil, baseCoverage, false) if err != nil { t.Fatalf("buildRollupPrompt base: %v", err) } - skippedPrompt, err := buildRollupPrompt(gitprovider.PR{}, findings, nil, skippedCoverage) + skippedPrompt, err := buildRollupPrompt(gitprovider.PR{}, findings, nil, skippedCoverage, false) if err != nil { t.Fatalf("buildRollupPrompt skipped: %v", err) } @@ -620,7 +652,7 @@ func TestRollupPromptBudgetUsesSynthesisModel(t *testing.T) { if rollupRuntime.model == req.SelectionModelOverride { t.Fatalf("rollup model = %q, want synthesis model independent of selection override", rollupRuntime.model) } - prompt, err := buildRollupPrompt(provider.pr, largeRollupFindings(4, "main.go", strings.Repeat("body ", 4000)), nil, nil) + prompt, err := buildRollupPrompt(provider.pr, largeRollupFindings(4, "main.go", strings.Repeat("body ", 4000)), nil, nil, false) if err != nil { t.Fatalf("buildRollupPrompt: %v", err) } @@ -640,7 +672,7 @@ func TestRollupPromptBudgetIgnoresSelectionModelOverride(t *testing.T) { if rollupRuntime.model == req.SelectionModelOverride { t.Fatalf("rollup model = %q, want synthesis model independent of selection override", rollupRuntime.model) } - prompt, err := buildRollupPrompt(provider.pr, largeRollupFindings(4, "main.go", strings.Repeat("body ", 4000)), nil, nil) + prompt, err := buildRollupPrompt(provider.pr, largeRollupFindings(4, "main.go", strings.Repeat("body ", 4000)), nil, nil, false) if err != nil { t.Fatalf("buildRollupPrompt: %v", err) } diff --git a/internal/pipeline/sensitive_value_rule_test.go b/internal/pipeline/sensitive_value_rule_test.go index 454dd42b..e6381760 100644 --- a/internal/pipeline/sensitive_value_rule_test.go +++ b/internal/pipeline/sensitive_value_rule_test.go @@ -30,7 +30,7 @@ func TestOutputContractsCarrySensitiveValueRule(t *testing.T) { {name: "selection with max agents", contract: selectionOutputContract(nil, []string{"main.go"}, nil, 2)}, {name: "findings", contract: findingsOutputContract("agent-1", []string{"main.go"})}, {name: "findings with relocations", contract: findingsOutputContractWithRelocations("agent-1", []string{"main.go"}, relocated)}, - {name: "rollup", contract: rollupOutputContract([]review.Finding{{ID: "finding-1"}})}, + {name: "rollup", contract: rollupOutputContract([]review.Finding{{ID: "finding-1"}}, false)}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/pipeline/thread_analysis_prompt_test.go b/internal/pipeline/thread_analysis_prompt_test.go index 3157eae2..349cef62 100644 --- a/internal/pipeline/thread_analysis_prompt_test.go +++ b/internal/pipeline/thread_analysis_prompt_test.go @@ -16,11 +16,11 @@ import ( func TestThreadAnalysisFailuresChangeSynthesisFingerprint(t *testing.T) { pr := gitprovider.PR{Title: "fixture"} - base, err := buildRollupPrompt(pr, nil, nil, nil) + base, err := buildRollupPrompt(pr, nil, nil, nil, false) if err != nil { t.Fatal(err) } - empty, err := buildRollupPrompt(pr, nil, nil, nil, nil) + empty, err := buildRollupPrompt(pr, nil, nil, nil, false, nil) if err != nil || empty != base || strings.Contains(base, "thread_analysis_failures") { t.Fatalf("no-failure prompt changed: base=%s empty=%s err=%v", base, empty, err) } @@ -33,7 +33,7 @@ func TestThreadAnalysisFailuresChangeSynthesisFingerprint(t *testing.T) { {ThreadID: "thread-2", Error: "structured output remained invalid after retry"}, {ThreadID: "thread-1", Error: "provider execution failed; see local task artifacts for details"}, } { - prompt, err := buildRollupPrompt(pr, nil, nil, nil, []reviewplan.ThreadAnalysisFailureSummary{failure}) + prompt, err := buildRollupPrompt(pr, nil, nil, nil, false, []reviewplan.ThreadAnalysisFailureSummary{failure}) if err != nil { t.Fatal(err) } @@ -54,7 +54,7 @@ func TestThreadFailureSynthesisPromptBoundsExamplesAndBindsOmittedEvidence(t *te failures[i] = reviewplan.ThreadAnalysisFailureSummary{ThreadID: fmt.Sprintf("thread-%04d", i), Error: strings.Repeat("界", 800)} } original := append([]reviewplan.ThreadAnalysisFailureSummary(nil), failures...) - prompt, err := buildRollupPrompt(gitprovider.PR{}, nil, nil, nil, failures) + prompt, err := buildRollupPrompt(gitprovider.PR{}, nil, nil, nil, false, failures) if err != nil { t.Fatal(err) } @@ -75,7 +75,7 @@ func TestThreadFailureSynthesisPromptBoundsExamplesAndBindsOmittedEvidence(t *te } changed := append([]reviewplan.ThreadAnalysisFailureSummary(nil), failures...) changed[5].ThreadID = "zz-omitted-thread-changed" - changedPrompt, err := buildRollupPrompt(gitprovider.PR{}, nil, nil, nil, changed) + changedPrompt, err := buildRollupPrompt(gitprovider.PR{}, nil, nil, nil, false, changed) if err != nil { t.Fatal(err) } @@ -96,7 +96,7 @@ func TestThreadFailureSynthesisPromptBoundsExamplesAndBindsOmittedEvidence(t *te } changed = append([]reviewplan.ThreadAnalysisFailureSummary(nil), failures...) changed[5].Error = "provider execution failed; see local task artifacts for details" - changedPrompt, err = buildRollupPrompt(gitprovider.PR{}, nil, nil, nil, changed) + changedPrompt, err = buildRollupPrompt(gitprovider.PR{}, nil, nil, nil, false, changed) if err != nil { t.Fatal(err) } @@ -107,7 +107,7 @@ func TestThreadFailureSynthesisPromptBoundsExamplesAndBindsOmittedEvidence(t *te for i, j := 0, len(reordered)-1; i < j; i, j = i+1, j-1 { reordered[i], reordered[j] = reordered[j], reordered[i] } - reorderedPrompt, err := buildRollupPrompt(gitprovider.PR{}, nil, nil, nil, reordered) + reorderedPrompt, err := buildRollupPrompt(gitprovider.PR{}, nil, nil, nil, false, reordered) if err != nil || reorderedPrompt != prompt { t.Fatalf("failure evidence digest is not deterministic across order, err=%v", err) } diff --git a/internal/review/review.go b/internal/review/review.go index 4522b4e8..3afc0805 100644 --- a/internal/review/review.go +++ b/internal/review/review.go @@ -374,11 +374,16 @@ type DedupeEntry struct { } // Rollup is the provider-neutral review rollup selected by the orchestrator. +// ReviewEvent is the orchestrator's proposal; reviewplan decides the posted +// event from finding severities and the review policy. type Rollup struct { ReviewEvent ReviewEvent `json:"review_event"` ReviewEventRationale string `json:"review_event_rationale"` DedupeLog []DedupeEntry `json:"dedupe_log"` OrderedFindings []FindingID `json:"ordered_findings"` + // Fallback marks a rollup built without valid orchestrator output: every + // finding is kept, with no deduplication and no proposed event. + Fallback bool `json:"fallback,omitempty"` } func normalizeLower(value string) string { diff --git a/internal/reviewplan/effective_event_test.go b/internal/reviewplan/effective_event_test.go index 438e648f..fa2db7ee 100644 --- a/internal/reviewplan/effective_event_test.go +++ b/internal/reviewplan/effective_event_test.go @@ -6,44 +6,54 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/review" ) -// Minor/nit-only findings must approve even when the rollup model proposes a -// comment — non-blocking, non-major findings are suggestions, not gates. -func TestEffectiveReviewEventMinorOnlyApproves(t *testing.T) { - minorOnly := []review.Finding{ - {Severity: review.SeverityMinor}, - {Severity: review.SeverityNits}, - } +func TestEffectiveReviewEvent(t *testing.T) { + blocking := []review.Finding{{Severity: review.SeverityMinor}, {Severity: review.SeverityBlocking}} + major := []review.Finding{{Severity: review.SeverityMinor}, {Severity: review.SeverityMajor}} + minorOnly := []review.Finding{{Severity: review.SeverityMinor}, {Severity: review.SeverityNits}} + commentPolicy := EventOptions{} + requestChangesPolicy := EventOptions{MajorEventRequestsChanges: true} + selfAuthored := EventOptions{PostingIdentityIsPRAuthor: true} + + const ( + unset = review.ReviewEvent("") + approve = review.ReviewEventApprove + comment = review.ReviewEventComment + requestChanges = review.ReviewEventRequestChanges + ) cases := []struct { - name string - rollup review.ReviewEvent + name string + findings []review.Finding + opts EventOptions + proposed review.ReviewEvent + want review.ReviewEvent }{ - {"rollup proposed comment", review.ReviewEventComment}, - {"rollup proposed request-changes", review.ReviewEventRequestChanges}, - {"rollup unset", ""}, + {"blocking unset", blocking, commentPolicy, unset, requestChanges}, + {"blocking approve", blocking, commentPolicy, approve, requestChanges}, + {"blocking comment", blocking, commentPolicy, comment, requestChanges}, + {"blocking comment under request_changes policy", blocking, requestChangesPolicy, comment, requestChanges}, + + {"major unset under comment policy", major, commentPolicy, unset, comment}, + {"major approve under comment policy", major, commentPolicy, approve, comment}, + {"major comment under comment policy", major, commentPolicy, comment, comment}, + {"major request_changes under comment policy", major, commentPolicy, requestChanges, comment}, + + {"major unset under request_changes policy", major, requestChangesPolicy, unset, requestChanges}, + {"major approve under request_changes policy", major, requestChangesPolicy, approve, requestChanges}, + {"major comment under request_changes policy", major, requestChangesPolicy, comment, comment}, + {"major request_changes under request_changes policy", major, requestChangesPolicy, requestChanges, requestChanges}, + + {"minor only unset", minorOnly, commentPolicy, unset, approve}, + {"minor only comment", minorOnly, commentPolicy, comment, approve}, + {"minor only request_changes", minorOnly, commentPolicy, requestChanges, approve}, + {"minor only request_changes when self-authored", minorOnly, selfAuthored, requestChanges, comment}, + + {"no findings", nil, commentPolicy, unset, approve}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - got := effectiveReviewEvent(tc.rollup, minorOnly, EventOptions{}) - if got != review.ReviewEventApprove { - t.Fatalf("got %q, want approve", got) + if got := effectiveReviewEvent(tc.proposed, tc.findings, tc.opts); got != tc.want { + t.Fatalf("effectiveReviewEvent = %q, want %q", got, tc.want) } }) } } - -// Blocking/major findings still govern: the clamp only forces approval, it never -// downgrades a genuine blocking/major signal. -func TestEffectiveReviewEventRespectsBlockingAndMajor(t *testing.T) { - withMajor := []review.Finding{{Severity: review.SeverityMinor}, {Severity: review.SeverityMajor}} - if got := effectiveReviewEvent(review.ReviewEventComment, withMajor, EventOptions{}); got != review.ReviewEventComment { - t.Fatalf("major present: got %q, want comment", got) - } - withBlocking := []review.Finding{{Severity: review.SeverityBlocking}} - if got := effectiveReviewEvent("", withBlocking, EventOptions{}); got != review.ReviewEventRequestChanges { - t.Fatalf("blocking present: got %q, want request-changes", got) - } - // An empty diff with no findings approves. - if got := effectiveReviewEvent("", nil, EventOptions{}); got != review.ReviewEventApprove { - t.Fatalf("no findings: got %q, want approve", got) - } -} diff --git a/internal/reviewplan/reviewplan.go b/internal/reviewplan/reviewplan.go index b84d3cb3..8fde60ac 100644 --- a/internal/reviewplan/reviewplan.go +++ b/internal/reviewplan/reviewplan.go @@ -350,22 +350,27 @@ func ReviewEventForFindings(findings []review.Finding, opts EventOptions) review } } -// effectiveReviewEvent resolves the posted review event. It begins with the -// rollup model's chosen event (or the severity-derived default when the model -// did not specify one), then clamps it so a review whose findings are all -// minor/nit — nothing blocking or major — always approves. Non-blocking, -// non-major findings are suggestions and must never gate approval, regardless -// of what the rollup model proposed. Blocking/major findings still govern as -// before (the model's choice, or the severity default, stands). +// effectiveReviewEvent resolves the posted review event from finding severities +// and the review policy. The rollup model proposes an event, but the proposal +// can only soften a major-only request_changes to a comment. It cannot approve +// over a blocking or major finding, cannot request changes against the policy, +// and cannot withhold approval from minor and nit findings, which are +// suggestions rather than gates. func effectiveReviewEvent(rollupEvent review.ReviewEvent, findings []review.Finding, opts EventOptions) review.ReviewEvent { - severityEvent := ReviewEventForFindings(findings, opts) - if severityEvent == review.ReviewEventApprove { - return review.ReviewEventApprove + event := ReviewEventForFindings(findings, opts) + if event == review.ReviewEventRequestChanges && rollupEvent == review.ReviewEventComment && !hasBlockingFinding(findings) { + return review.ReviewEventComment } - if rollupEvent == "" { - return severityEvent + return event +} + +func hasBlockingFinding(findings []review.Finding) bool { + for _, finding := range findings { + if finding.Severity == review.SeverityBlocking { + return true + } } - return rollupEvent + return false } func applySelfApprovalPolicy(event review.ReviewEvent, opts EventOptions) review.ReviewEvent { @@ -478,10 +483,10 @@ func (b *builder) buildReview() (Plan, error) { if err != nil { return Plan{}, err } - event := effectiveReviewEvent(b.req.Rollup.ReviewEvent, ordered, b.req.EventOptions) - if !event.Valid() { - return Plan{}, fmt.Errorf("reviewplan: invalid rollup review event %q", event) + if proposed := b.req.Rollup.ReviewEvent; proposed != "" && !proposed.Valid() { + return Plan{}, fmt.Errorf("reviewplan: invalid rollup review event %q", proposed) } + event := effectiveReviewEvent(b.req.Rollup.ReviewEvent, ordered, b.req.EventOptions) event = applySelfApprovalPolicy(event, b.req.EventOptions) approvalWithheld := false if event == review.ReviewEventApprove && (len(b.req.RunSummary.ReviewerFailures) > 0 || len(b.req.RunSummary.ThreadAnalysisFailures) > 0 || hasIncompleteReviewerCoverage(b.req.RunSummary.ReviewerCoverage)) { @@ -1018,6 +1023,10 @@ func (b *builder) renderRollup(ordered []review.Finding, anchored []AnchoredFind } writeThreadAnalysisDiagnostics(&out, summary.Run.ThreadAnalysisFailures) writeThreadCountsLine(&out, summary.Threads) + if b.req.Rollup.Fallback { + out.WriteString("\n---\n\n") + out.WriteString("> Note: The rollup step did not return valid output, so duplicate findings were not merged. The review event comes from finding severities and the review policy.\n") + } if b.req.AgentDefinitionsChanged { out.WriteString("\n---\n\n") out.WriteString("> Note: This PR modifies reviewer definitions under `.codereview/agents/`. The review was conducted using base-branch versions; changes will affect future reviews after merge.\n") diff --git a/internal/reviewplan/summary_bounded_test.go b/internal/reviewplan/summary_bounded_test.go index 16d65878..cb12093e 100644 --- a/internal/reviewplan/summary_bounded_test.go +++ b/internal/reviewplan/summary_bounded_test.go @@ -32,6 +32,7 @@ func TestReviewerCoverageDiagnosticsBoundLargeCollections(t *testing.T) { original := cloneReviewerCoverage(coverage) req := summaryRequest() + req.EventOptions.MajorEventRequestsChanges = true req.RunSummary.ReviewerCoverage = coverage plan, err := Build(req) if err != nil { From e075c50208d489055be5e81d25f1df92ae55553b Mon Sep 17 00:00:00 2001 From: piekstra Date: Tue, 3 Nov 2026 06:46:08 -0500 Subject: [PATCH 2/2] test(reviewplan): cover validation of the proposed rollup event Twapp-Session: 59d912f3-982f-402e-8bd8-3e31269a28b0 --- internal/reviewplan/effective_event_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/internal/reviewplan/effective_event_test.go b/internal/reviewplan/effective_event_test.go index fa2db7ee..9bffe3e6 100644 --- a/internal/reviewplan/effective_event_test.go +++ b/internal/reviewplan/effective_event_test.go @@ -1,6 +1,7 @@ package reviewplan import ( + "strings" "testing" "github.com/open-cli-collective/codereview-cli/internal/review" @@ -57,3 +58,17 @@ func TestEffectiveReviewEvent(t *testing.T) { }) } } + +func TestBuildValidatesProposedRollupEvent(t *testing.T) { + unknown := summaryRequest() + unknown.Rollup.ReviewEvent = "merge" + if _, err := Build(unknown); err == nil || !strings.Contains(err.Error(), "invalid rollup review event") { + t.Fatalf("Build with unknown proposal error = %v, want invalid rollup review event", err) + } + + unset := summaryRequest() + unset.Rollup.ReviewEvent = "" + if _, err := Build(unset); err != nil { + t.Fatalf("Build with no proposal: %v", err) + } +}