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

docs/checkout-native-review-contract.md

@@ -336,9 +336,19 @@ silently.
 
 Coverage uses two related scopes:
 
-- readable files: all changed files in the workbench
+- readable files: all changed files in the workbench, **except generated
+  dependency lockfiles** (`Cargo.lock`, `package-lock.json`, `go.sum`, and the
+  well-known peers — see `isGeneratedLockfile`). Lockfiles are machine-written
+  and reviewed, if at all, through the manifest change that produced them, so
+  they are exempt from the coverage universe: neither a reviewer that skips one
+  nor an unassigned lockfile counts as incomplete coverage. The exemption is
+  applied in one place — the orchestrator's glob-coverage assigner does not
+  force-assign a lockfile, and the coverage accounting drops lockfiles from both
+  scope and inspected/skipped rows — so scope and coverage are drawn from the
+  same set.
 - assignment scope: `allowed_files` when present, otherwise `files` when the
-  orchestrator supplied them, otherwise all changed files
+  orchestrator supplied them, otherwise all changed files (lockfiles exempt, as
+  above)
 
 The coverage status values are:
 

internal/llm/contracts.go

@@ -37,8 +37,10 @@ type FindingsConstraintLimits struct {
 	MaxRunesPerEntry int
 }
 
-// DefaultFindingsConstraintLimits returns the fixed reviewer-constraint
-// limits enforced by DecodeFindings.
+// DefaultFindingsConstraintLimits returns the fixed reviewer-constraint limits.
+// DecodeFindings applies them by capping the count and truncating over-long
+// entries (see decodeCoverageConstraints), not by failing — constraints are
+// informational, so a verbose one must not sink the reviewer.
 func DefaultFindingsConstraintLimits() FindingsConstraintLimits {
 	return FindingsConstraintLimits{
 		MaxEntries:       defaultMaxCoverageConstraints,
@@ -272,10 +274,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 := decodeCoverageConstraints(wire.Constraints)
 
 	result := Findings{
 		AgentID:        wire.AgentID,
@@ -352,27 +351,34 @@ func decodeCoverageFiles(name string, files []string, changedFiles map[string]bo
 	return out, nil
 }
 
-func decodeCoverageStrings(name string, values []string) ([]string, error) {
+// decodeCoverageConstraints 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 decodeCoverageConstraints(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)
-		}
+		// Sanitize first (it can rewrite/grow a marker), then clamp the final
+		// length so the result always fits the per-entry cap. truncateRunes
+		// appends a 3-rune "...", so reserve that width.
 		value = sanitize(value)
-		if strings.TrimSpace(value) == "" {
-			return nil, fmt.Errorf("llm: %s entries must be non-empty", name)
+		if utf8.RuneCountInString(value) > defaultMaxCoverageConstraintRunes {
+			value = truncateRunes(value, defaultMaxCoverageConstraintRunes-3)
 		}
-		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,82 @@ 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])
+			}
+			// Truncation (ellipsis included) must stay within the documented cap.
+			if n := utf8.RuneCountInString(got.Constraints[0]); n > limits.MaxRunesPerEntry {
+				t.Fatalf("constraint = %d runes, want ≤ %d (ellipsis must fit the cap)", n, limits.MaxRunesPerEntry)
+			}
 		})
 	}
 }
 
+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

@@ -1576,6 +1576,12 @@ func ensureSelectedGlobCoverage(selection llm.Selection, catalog agents.Catalog,
 		if covered[file] {
 			continue
 		}
+		// Generated lockfiles are not a coverage obligation (see
+		// buildReviewerCoverage): don't force-assign one to an agent, or its
+		// prompt scope would list a file the accounting layer then exempts.
+		if isGeneratedLockfile(file) {
+			continue
+		}
 		for i := range selection.SelectedAgents {
 			selected := &selection.SelectedAgents[i]
 			candidate, ok := agentByID[selected.AgentID]
@@ -2437,10 +2443,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 +2504,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
 		}
@@ -2473,7 +2527,10 @@ func buildReviewerCoverage(selected []llm.SelectedAgent, results []llm.Findings,
 			out = append(out, entry)
 			continue
 		}
-		entry.InspectedFiles = copySortedStrings(result.InspectedFiles)
+		// Draw both scope and coverage rows from the same lockfile-exempt set,
+		// so a reviewer that did read a lockfile doesn't emit an inspected file
+		// outside its scope.
+		entry.InspectedFiles = filterReviewableFiles(copySortedStrings(result.InspectedFiles))
 		entry.SkippedFiles = sortedIntersection(result.SkippedFiles, scope)
 		entry.Constraints = copySortedStrings(result.Constraints)
 		if evidence := reviewerToolEvidenceForAgent(toolEvidence, agent.AgentID); evidence != nil && evidence.DiffStatus != llm.DiffToolStatusSucceeded {

internal/pipeline/pipeline_test.go

@@ -5097,6 +5097,62 @@ 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.
+
+	// A reviewer that reports *inspecting* a lockfile: it must be dropped from
+	// the coverage row too, so scope and inspected files come from one set.
+	inspectedLock := buildReviewerCoverage(
+		[]llm.SelectedAgent{{AgentID: "rust:impl", Files: []string{"main.go"}}},
+		[]llm.Findings{{AgentID: "rust:impl", InspectedFiles: []string{"Cargo.lock", "main.go"}}},
+		nil,
+		[]string{"main.go", "Cargo.lock"},
+	)
+	if len(inspectedLock) != 1 || !reflect.DeepEqual(inspectedLock[0].InspectedFiles, []string{"main.go"}) {
+		t.Fatalf("inspected files = %#v, want [main.go] with the lockfile dropped", inspectedLock)
+	}
+}
+
+func TestEnsureSelectedGlobCoverageSkipsLockfiles(t *testing.T) {
+	// A changed lockfile that matches an agent's globs must NOT be force-assigned
+	// into that agent's scope — it is exempt from the coverage universe. Without
+	// the skip the reviewer would be told to cover Cargo.lock, which the
+	// accounting layer then exempts, reintroducing the split this fix removes.
+	catalog := agents.Catalog{Agents: []agents.Agent{
+		{ID: "rust:impl", FileGlobs: []string{"**/*.lock", "**/*.rs"}},
+	}}
+	selection := llm.Selection{SelectedAgents: []llm.SelectedAgent{
+		{AgentID: "rust:impl", Files: []string{"main.rs"}},
+	}}
+	got := ensureSelectedGlobCoverage(selection, catalog, []string{"main.rs", "Cargo.lock"})
+	for _, f := range got.SelectedAgents[0].Files {
+		if f == "Cargo.lock" {
+			t.Fatalf("Cargo.lock was force-assigned into agent scope: %#v", got.SelectedAgents[0].Files)
+		}
+	}
+}
+
 func TestBuildReviewerCoverageUsesTypedToolEvidenceInsteadOfModelConstraint(t *testing.T) {
 	got := buildReviewerCoverage(
 		[]llm.SelectedAgent{{AgentID: "harness:reviewer", Files: []string{"main.go"}}},