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..9bffe3e6 100644 --- a/internal/reviewplan/effective_event_test.go +++ b/internal/reviewplan/effective_event_test.go @@ -1,49 +1,74 @@ package reviewplan import ( + "strings" "testing" "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) +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) } - 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) + + unset := summaryRequest() + unset.Rollup.ReviewEvent = "" + if _, err := Build(unset); err != nil { + t.Fatalf("Build with no proposal: %v", err) } } 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 {