diff --git a/internal/llm/contracts.go b/internal/llm/contracts.go index ef70e2b..9082eb8 100644 --- a/internal/llm/contracts.go +++ b/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 { diff --git a/internal/llm/contracts_test.go b/internal/llm/contracts_test.go index d2e4f3a..dc7c1a5 100644 --- a/internal/llm/contracts_test.go +++ b/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{ diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index cbabd0a..7b147c0 100644 --- a/internal/pipeline/pipeline.go +++ b/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 } diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index e682619..12c9546 100644 --- a/internal/pipeline/pipeline_test.go +++ b/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"}}},