fix: make reviewer coverage resilient (exempt lockfiles; degrade over-long constraints)

internal/llm/contracts.go

@@ -272,10 +272,7 @@ func DecodeFindings(data []byte, opts FindingsOptions) (Findings, error) {
 	if err := validateCoverageFileDisjoint(inspected, skipped); err != nil {
 		return Findings{}, err
 	}
-	constraints, err := decodeCoverageStrings("constraints", wire.Constraints)
-	if err != nil {
-		return Findings{}, err
-	}
+	constraints := decodeCoverageStrings(wire.Constraints)
 
 	result := Findings{
 		AgentID:        wire.AgentID,
@@ -352,27 +349,31 @@ func decodeCoverageFiles(name string, files []string, changedFiles map[string]bo
 	return out, nil
 }
 
-func decodeCoverageStrings(name string, values []string) ([]string, error) {
+// decodeCoverageStrings cleans reviewer coverage constraints. These are
+// informational notes ("couldn't verify X against source-of-truth docs"), not
+// a contract, so a malformed or verbose entry is degraded — the count is
+// capped, an over-long entry is truncated, and empties/duplicates are dropped —
+// rather than failing the decode. Failing here sinks the whole reviewer as
+// "completed without a result file" and blocks approval on an otherwise-clean
+// review, which a single legitimate ~300-rune constraint once did.
+func decodeCoverageStrings(values []string) []string {
 	if len(values) > defaultMaxCoverageConstraints {
-		return nil, fmt.Errorf("llm: %s cap exceeded", name)
+		values = values[:defaultMaxCoverageConstraints]
 	}
 	out := make([]string, 0, len(values))
 	seen := map[string]bool{}
 	for _, value := range values {
 		if utf8.RuneCountInString(value) > defaultMaxCoverageConstraintRunes {
-			return nil, fmt.Errorf("llm: %s entry length out of bounds", name)
+			value = truncateRunes(value, defaultMaxCoverageConstraintRunes)
 		}
 		value = sanitize(value)
-		if strings.TrimSpace(value) == "" {
-			return nil, fmt.Errorf("llm: %s entries must be non-empty", name)
-		}
-		if seen[value] {
-			return nil, fmt.Errorf("llm: duplicate %s entry %q", name, value)
+		if strings.TrimSpace(value) == "" || seen[value] {
+			continue
 		}
 		seen[value] = true
 		out = append(out, value)
 	}
-	return out, nil
+	return out
 }
 
 func validateCoverageFileDisjoint(inspected, skipped []string) error {

internal/llm/contracts_test.go

@@ -155,8 +155,6 @@ func TestDecodeFindings(t *testing.T) {
 	assertFindingsError(t, baseOpts, `{"schema_version":1,"agent_id":"agent-1","inspected_files":["main.go","main.go"],"findings":[]}`, "duplicate inspected_files")
 	assertFindingsError(t, baseOpts, `{"schema_version":1,"agent_id":"agent-1","inspected_files":["main.go"],"skipped_files":["other.go"],"findings":[]}`, "skipped_files entry")
 	assertFindingsError(t, baseOpts, `{"schema_version":1,"agent_id":"agent-1","inspected_files":["main.go"],"skipped_files":["main.go"],"findings":[]}`, "both inspected and skipped")
-	assertFindingsError(t, baseOpts, `{"schema_version":1,"agent_id":"agent-1","inspected_files":["main.go"],"constraints":["  "],"findings":[]}`, "constraints")
-	assertFindingsError(t, baseOpts, `{"schema_version":1,"agent_id":"agent-1","inspected_files":["main.go"],"constraints":["one","two","three","four","five","six","seven","eight","nine","ten","eleven"],"findings":[]}`, "constraints cap exceeded")
 	assertFindingsError(t, baseOpts, findingsFixture(`"schema_version":2,"agent_id":"agent-1","findings":[]`), "schema_version")
 	assertFindingsError(t, baseOpts, findingsFixture(`"schema_version":1,"agent_id":"agent-1","findings":[],"extra":true`), "unknown field")
 	assertFindingsError(t, baseOpts, findingsFixture(`"schema_version":1,"agent_id":"missing","findings":[]`), "unknown findings agent")
@@ -187,35 +185,78 @@ func TestDecodeFindingsConstraintRuneBoundaries(t *testing.T) {
 	multibyteAtLimit := strings.Repeat("界", limits.MaxRunesPerEntry)
 
 	for _, tt := range []struct {
-		name       string
-		constraint string
-		wantErr    string
-		wantClean  bool
+		name          string
+		constraint    string
+		wantTruncated bool
 	}{
-		{name: "marker opening at limit", constraint: markerAtLimit, wantClean: true},
-		{name: "marker opening over limit", constraint: markerAtLimit + "x", wantErr: "constraints entry length"},
+		{name: "marker opening at limit", constraint: markerAtLimit},
+		// Over-limit entries are truncated, not rejected: a verbose (but valid)
+		// coverage note must not fail the whole reviewer and block approval.
+		{name: "marker opening over limit", constraint: markerAtLimit + "x", wantTruncated: true},
 		{name: "multibyte at limit", constraint: multibyteAtLimit},
-		{name: "multibyte over limit", constraint: multibyteAtLimit + "界", wantErr: "constraints entry length"},
+		{name: "multibyte over limit", constraint: multibyteAtLimit + "界", wantTruncated: true},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
 			got, err := decodeFindingsWithConstraint(t, tt.constraint)
-			if tt.wantErr != "" {
-				assertErrContains(t, err, tt.wantErr)
-				return
-			}
 			if err != nil {
 				t.Fatalf("DecodeFindings: %v", err)
 			}
 			if len(got.Constraints) != 1 {
 				t.Fatalf("constraints = %#v, want one value", got.Constraints)
 			}
-			if tt.wantClean && strings.Contains(got.Constraints[0], markerOpening) {
+			if strings.Contains(got.Constraints[0], markerOpening) {
 				t.Fatalf("constraint = %q, want sanitized marker opening", got.Constraints[0])
 			}
+			if tt.wantTruncated && !strings.HasSuffix(got.Constraints[0], "...") {
+				t.Fatalf("constraint = %q, want truncated (ends with ...), not rejected", got.Constraints[0])
+			}
 		})
 	}
 }
 
+func TestDecodeFindingsConstraintsDegradeInsteadOfFailing(t *testing.T) {
+	// Coverage constraints are informational: an over-count list, a
+	// whitespace-only entry, and a duplicate are cleaned rather than failing
+	// the whole reviewer (which surfaced as "completed without a result file"
+	// and blocked approval on an otherwise-clean review).
+	payload := map[string]any{
+		"schema_version":  1,
+		"agent_id":        "agent-1",
+		"inspected_files": []string{"main.go"},
+		"constraints": []string{
+			"a", "  ", "b", "a", // whitespace-only and duplicate, within the cap
+			"c", "d", "e", "f", "g", "h", "i", "j", // pushes the list over the cap of 10
+		},
+		"findings": []any{},
+	}
+	data, err := json.Marshal(payload)
+	if err != nil {
+		t.Fatalf("Marshal: %v", err)
+	}
+	got, err := DecodeFindings(data, FindingsOptions{
+		KnownAgents:  map[string]bool{"agent-1": true},
+		ChangedFiles: map[string]bool{"main.go": true},
+		NewFindingID: newIDQueue("f-1").next,
+	})
+	if err != nil {
+		t.Fatalf("DecodeFindings degraded to an error: %v", err)
+	}
+	lim := DefaultFindingsConstraintLimits()
+	if len(got.Constraints) > lim.MaxEntries {
+		t.Fatalf("constraints = %#v, want ≤ %d after capping", got.Constraints, lim.MaxEntries)
+	}
+	seen := map[string]bool{}
+	for _, c := range got.Constraints {
+		if strings.TrimSpace(c) == "" {
+			t.Fatalf("kept a whitespace-only constraint: %#v", got.Constraints)
+		}
+		if seen[c] {
+			t.Fatalf("kept a duplicate constraint: %#v", got.Constraints)
+		}
+		seen[c] = true
+	}
+}
+
 func decodeFindingsWithConstraint(t *testing.T, constraint string) (Findings, error) {
 	t.Helper()
 	payload := map[string]any{

internal/pipeline/pipeline.go

@@ -2437,10 +2437,56 @@ func reviewerToolEvidenceByAgent(sessions []sessionDraft) map[string]*llm.Review
 	return out
 }
 
+// generatedLockfiles are dependency lockfiles: machine-written by a package
+// manager and reviewed (if at all) through the manifest change that produced
+// them, never line by line. A reviewer that skips one is behaving correctly, so
+// they are excluded from the coverage universe — otherwise a skipped lockfile
+// marks the reviewer incomplete_skipped and blocks approval on an otherwise
+// clean review. (A real PR stalled exactly this way: a Cargo.lock churned by a
+// dependency bump was the only file left "unreviewed".)
+var generatedLockfiles = map[string]bool{
+	"Cargo.lock":          true,
+	"package-lock.json":   true,
+	"npm-shrinkwrap.json": true,
+	"yarn.lock":           true,
+	"pnpm-lock.yaml":      true,
+	"bun.lockb":           true,
+	"go.sum":              true,
+	"Gemfile.lock":        true,
+	"poetry.lock":         true,
+	"Pipfile.lock":        true,
+	"composer.lock":       true,
+	"Podfile.lock":        true,
+	"flake.lock":          true,
+	"mix.lock":            true,
+}
+
+// isGeneratedLockfile reports whether path is a dependency lockfile a reviewer
+// is not expected to read line by line.
+func isGeneratedLockfile(path string) bool {
+	return generatedLockfiles[filepath.Base(path)]
+}
+
+// filterReviewableFiles drops generated lockfiles from a file list so they do
+// not become a coverage obligation.
+func filterReviewableFiles(files []string) []string {
+	out := make([]string, 0, len(files))
+	for _, file := range files {
+		if isGeneratedLockfile(file) {
+			continue
+		}
+		out = append(out, file)
+	}
+	return out
+}
+
 func buildReviewerCoverage(selected []llm.SelectedAgent, results []llm.Findings, failures []ReviewerFailure, changedFiles []string, toolEvidence ...map[string]*llm.ReviewerToolEvidence) []reviewplan.ReviewerCoverageSummary {
 	if len(selected) == 0 && len(changedFiles) == 0 {
 		return nil
 	}
+	// Generated lockfiles are not a review obligation: exclude them so neither a
+	// reviewer that skips one nor an unassigned lockfile blocks approval.
+	changedFiles = filterReviewableFiles(changedFiles)
 	resultByAgent := make(map[string]llm.Findings, len(results))
 	for _, result := range results {
 		resultByAgent[result.AgentID] = result
@@ -2452,7 +2498,9 @@ func buildReviewerCoverage(selected []llm.SelectedAgent, results []llm.Findings,
 	assigned := map[string]bool{}
 	out := make([]reviewplan.ReviewerCoverageSummary, 0, len(selected)+1)
 	for _, agent := range selected {
-		scope := reviewerAssignmentScope(agent, changedFiles)
+		// A lockfile explicitly assigned to an agent is exempt too — the scope
+		// is what the reviewer is held to, and lockfiles are not reviewable.
+		scope := filterReviewableFiles(reviewerAssignmentScope(agent, changedFiles))
 		for _, file := range scope {
 			assigned[file] = true
 		}

internal/pipeline/pipeline_test.go

@@ -5097,6 +5097,31 @@ func TestBuildReviewerCoverageStatuses(t *testing.T) {
 	}
 }
 
+func TestBuildReviewerCoverageExemptsGeneratedLockfiles(t *testing.T) {
+	// A reviewer that inspects the real change and skips only the churned
+	// Cargo.lock is complete, not incomplete_skipped — a lockfile is not a
+	// review obligation. An unassigned lockfile (yarn.lock) likewise must not
+	// surface as incomplete_unassigned and block approval.
+	selected := []llm.SelectedAgent{
+		{AgentID: "rust:impl", Files: []string{"main.go", "Cargo.lock"}, AllowedFiles: []string{"main.go", "Cargo.lock"}},
+	}
+	results := []llm.Findings{
+		{AgentID: "rust:impl", InspectedFiles: []string{"main.go"}, SkippedFiles: []string{"Cargo.lock"}},
+	}
+	got := buildReviewerCoverage(selected, results, nil, []string{"main.go", "Cargo.lock", "yarn.lock"})
+	if len(got) != 1 {
+		t.Fatalf("coverage = %#v, want a single reviewer entry (no lockfile coverage rows)", got)
+	}
+	if got[0].AgentID != "rust:impl" || got[0].Status != reviewerCoverageCompleteConstrained {
+		t.Fatalf("coverage = %#v, want rust:impl complete_constrained", got)
+	}
+	if len(got[0].SkippedFiles) != 0 {
+		t.Fatalf("skipped files = %#v, want none (Cargo.lock is exempt from coverage)", got[0].SkippedFiles)
+	}
+	// complete_constrained is an approvable status; had Cargo.lock counted, this
+	// would be reviewerCoverageIncompleteSkipped, which blocks approval.
+}
+
 func TestBuildReviewerCoverageUsesTypedToolEvidenceInsteadOfModelConstraint(t *testing.T) {
 	got := buildReviewerCoverage(
 		[]llm.SelectedAgent{{AgentID: "harness:reviewer", Files: []string{"main.go"}}},