fix(review): post findings when the rollup conflicts with the review policy

README.md

@@ -681,7 +681,7 @@ Supported values:
 | `llm.model_map` keys | `small`, `medium`, `large` |
 | `llm_runtimes.<name>.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. |

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

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

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)
+		})
 	}
 }
 

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
 }