fix: make reviewer coverage resilient (exempt lockfiles; degrade over-long constraints) - #567
Conversation
A reviewer that skips a dependency lockfile (Cargo.lock, package-lock.json, go.sum, …) was marked incomplete_skipped, and hasIncompleteReviewerCoverage then downgraded an otherwise-clean APPROVE to a COMMENT — permanently, since the reviewer will always skip a machine-generated lockfile. Exempt lockfiles from the coverage universe (they are reviewed, if at all, via the manifest change that produced them, never line by line) so neither a skipped nor an unassigned lockfile blocks approval. Seen in the wild: a Cargo.lock churned by a v0.4→v0.5 dependency bump was the only 'unreviewed' file on a clean PR, and cr would not approve it.
… failing the reviewer
A reviewer whose coverage `constraints` entry exceeded 300 runes (or exceeded
the count cap, or was empty/duplicate) failed DecodeFindings with 'entry length
out of bounds', which surfaced downstream as 'completed without a result file'
and sank the whole reviewer — blocking approval on an otherwise-clean review. A
single legitimate ~300-rune note ('could not verify against source-of-truth
docs not in context') did exactly that, repeatably.
Coverage constraints are informational, not a contract: cap the count, truncate
an over-long entry, and drop empties/duplicates rather than erroring. Tests
updated to assert graceful degradation.
piekstra
changed the title
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 6d0772e32c08
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| structure:repo-health | 0 |
| harness-engineering:repo-health | 1 |
| architecture:solid | 2 |
go:implementation-tests (1 finding)
Minor - internal/llm/contracts.go:367
decodeCoverageStrings truncates an over-long constraint to defaultMaxCoverageConstraintRunes (300) and then appends "..." via truncateRunes, so the value that survives sanitize() can be up to 303 runes — 3 runes past the documented/enforced cap (DefaultFindingsConstraintLimits().MaxRunesPerEntry = 300, which the orchestrator prompt tells reviewers is the hard limit). TestDecodeFindingsConstraintRuneBoundaries only asserts the result ends with "...", not that it respects the rune cap, so this drift isn't caught. Fix by reserving the ellipsis width before truncating, e.g. truncateRunes(value, defaultMaxCoverageConstraintRunes-3), or by post-truncating the whole result (including the suffix) down to defaultMaxCoverageConstraintRunes.
harness-engineering:repo-health (1 finding)
Major - internal/pipeline/pipeline.go:2437
docs/checkout-native-review-contract.md is the documented source of truth for reviewer coverage semantics: it defines "readable files" as "all changed files in the workbench" and states unassigned/skipped files "must not turn into a clean approval silently." This diff's buildReviewerCoverage now filters generated lockfiles out of changedFiles (and each agent's scope) before computing coverage, so lockfiles are silently excluded from the readable-files universe the doc describes — but the doc's coverage-status section (lines ~337-354) was not updated to mention the lockfile exemption or the new filterReviewableFiles/isGeneratedLockfile step. A future agent or engineer reading that doc to understand what "complete" coverage guarantees will get a materially misleading picture of what the harness actually enforces. Update docs/checkout-native-review-contract.md to document the lockfile exemption alongside the existing coverage-status definitions.
architecture:solid (2 findings)
Minor - internal/pipeline/pipeline.go:2489
U-S1: the exemption is applied at the accounting layer only, so the repo now answers "what is in the coverage universe?" in two places that disagree.
ensureSelectedGlobCoverage(pipeline.go:1561-1594) still iterates the unfilteredchangedFilesand force-assigns an uncovered lockfile to the first glob-matching agent, widening that agent'sAllowedFiles/Files. The reviewer is therefore still told to coverCargo.lockin its prompt scope, and the obligation is then silently discarded here. A smaller observable version of the same split:entry.Scopeis filtered (line 2503) butentry.InspectedFiles(line 2524) is not, so a reviewer that did read a lockfile emits a coverage row listing an inspected file that is not in its scope.Choosing the accounting layer is the right call — filtering
prepared.changedFileswholesale would also drop lockfiles fromFindingsOptions.ChangedFilesand make a legitimate finding on a swapped registry URL undecodable — so the fix is not to move the filter but to give the universe one owner. Concretely: export the predicate as a single helper used both here and inensureSelectedGlobCoverage'scovered/assignment loops (e.g. have that function skip files whereisGeneratedLockfile(file)before looking for an owner), and apply the same filter toentry.InspectedFilesso scope and coverage rows are drawn from one set. Without that, the next place that computes an obligation from rawchangedFilesreintroduces the stall this PR fixes.
Nits - internal/llm/contracts.go:359
U-L2 (naming, not behavior): the degrade-don't-reject decision is sound and explicitly commented, so the swallowed-error rule is satisfied. Two labels are now stale around it.
decodeCoverageStringssits next todecodeCoverageFiles(line 335) with a near-identical name and the opposite error contract — one rejects, one silently repairs;decodeCoverageConstraintswould carry the distinction, since constraints are its only caller (line 275). AndDefaultFindingsConstraintLimits's doc comment still reads "limits enforced by DecodeFindings" (line 41), which now overstates: entries are capped and truncated, never enforced by failure. Prompt text at prompts.go:716-717 can keep saying "must" — instructing the model harder than the validator fails is deliberate.
Reviewer Coverage
go:implementation-tests— complete (broad); skipped: none; constraints: nonestructure:repo-health— complete (broad); inspected 2 assigned files (4 inspected across reviewers):internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go; skipped: none; constraints: Could not run git diff between base and head SHAs (git commands required interactive approval unavailable in this session); reviewed the head-checkout source directly instead of the exact unified diff.harness-engineering:repo-health— complete (broad); inspected 2 assigned files (4 inspected across reviewers):internal/llm/contracts.go,internal/pipeline/pipeline.go; skipped: none; constraints: nonearchitecture:solid— complete (broad); inspected 2 assigned files (4 inspected across reviewers):internal/llm/contracts.go,internal/pipeline/pipeline.go; skipped: none; constraints: Reviewed only the two assigned files; contracts_test.go and pipeline_test.go were read for intent but not reviewed as changed files. Sandbox blocked git in the reviewer checkout, so the diff was reconstructed from the head-of-branch files plus the change map rather than read as a patch; changed-line boundaries in contracts.go are inferred. Sandbox blocked running the project's verification (go test ./internal/llm/ ./internal/pipeline/, go vet); findings rest on reading, not on a reproduced build or test run.
Inspected files (4)
internal/llm/contracts.gointernal/llm/contracts_test.gointernal/pipeline/pipeline.gointernal/pipeline/pipeline_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 6m 07s | $4.91 | claude-sonnet-5, claude-opus-5 | cr dev
| Field | Value |
|---|---|
| Model | claude-sonnet-5, claude-opus-5 |
| Reviewers | go:implementation-tests, structure:repo-health, harness-engineering:repo-health, architecture:solid |
| Engine | claude_cli · claude-sonnet-5, claude-opus-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 6m 07s wall · 13m 14s compute |
| Cost | $4.91 |
| Tokens | 208 in / 54.6k out |
Per-workstream usage
orchestrator-selection— claude-sonnet-5- In: 6
- Out: 2.8k
- Cache read: 66.9k
- Cache create: 18.1k
- Cost: $0.17
- Duration: 30s
go:implementation-tests— claude-sonnet-5- In: 48
- Out: 12.4k
- Cache read: 1.3M
- Cache create: 64.6k
- Cost: $0.96
- Duration: 3m 02s
structure:repo-health— claude-sonnet-5- In: 58
- Out: 12.5k
- Cache read: 1.3M
- Cache create: 56.5k
- Cost: $0.91
- Duration: 3m 01s
harness-engineering:repo-health— claude-sonnet-5- In: 28
- Out: 6.2k
- Cache read: 680.9k
- Cache create: 57.1k
- Cost: $0.64
- Duration: 1m 15s
architecture:solid— claude-opus-5- In: 62
- Out: 19.9k
- Cache read: 1.4M
- Cache create: 80.9k
- Cost: $2.00
- Duration: 4m 49s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 882
- Cache read: 100.4k
- Cache create: 30.7k
- Cost: $0.23
- Duration: 36s
| } | ||
| // 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) |
There was a problem hiding this comment.
U-S1: the exemption is applied at the accounting layer only, so the repo now answers "what is in the coverage universe?" in two places that disagree. ensureSelectedGlobCoverage (pipeline.go:1561-1594) still iterates the unfiltered changedFiles and force-assigns an uncovered lockfile to the first glob-matching agent, widening that agent's AllowedFiles/Files. The reviewer is therefore still told to cover Cargo.lock in its prompt scope, and the obligation is then silently discarded here. A smaller observable version of the same split: entry.Scope is filtered (line 2503) but entry.InspectedFiles (line 2524) is not, so a reviewer that did read a lockfile emits a coverage row listing an inspected file that is not in its scope.
Choosing the accounting layer is the right call — filtering prepared.changedFiles wholesale would also drop lockfiles from FindingsOptions.ChangedFiles and make a legitimate finding on a swapped registry URL undecodable — so the fix is not to move the filter but to give the universe one owner. Concretely: export the predicate as a single helper used both here and in ensureSelectedGlobCoverage's covered/assignment loops (e.g. have that function skip files where isGeneratedLockfile(file) before looking for an owner), and apply the same filter to entry.InspectedFiles so scope and coverage rows are drawn from one set. Without that, the next place that computes an obligation from raw changedFiles reintroduces the stall this PR fixes.
Reply inline to this comment.
| // 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 { |
There was a problem hiding this comment.
U-L2 (naming, not behavior): the degrade-don't-reject decision is sound and explicitly commented, so the swallowed-error rule is satisfied. Two labels are now stale around it. decodeCoverageStrings sits next to decodeCoverageFiles (line 335) with a near-identical name and the opposite error contract — one rejects, one silently repairs; decodeCoverageConstraints would carry the distinction, since constraints are its only caller (line 275). And DefaultFindingsConstraintLimits's doc comment still reads "limits enforced by DecodeFindings" (line 41), which now overstates: entries are capped and truncated, never enforced by failure. Prompt text at prompts.go:716-717 can keep saying "must" — instructing the model harder than the validator fails is deliberate.
Reply inline to this comment.
| @@ -2437,10 +2437,56 @@ func reviewerToolEvidenceByAgent(sessions []sessionDraft) map[string]*llm.Review | |||
| return out | |||
There was a problem hiding this comment.
File-level note: internal/pipeline/pipeline.go
docs/checkout-native-review-contract.md is the documented source of truth for reviewer coverage semantics: it defines "readable files" as "all changed files in the workbench" and states unassigned/skipped files "must not turn into a clean approval silently." This diff's buildReviewerCoverage now filters generated lockfiles out of changedFiles (and each agent's scope) before computing coverage, so lockfiles are silently excluded from the readable-files universe the doc describes — but the doc's coverage-status section (lines ~337-354) was not updated to mention the lockfile exemption or the new filterReviewableFiles/isGeneratedLockfile step. A future agent or engineer reading that doc to understand what "complete" coverage guarantees will get a materially misleading picture of what the harness actually enforces. Update docs/checkout-native-review-contract.md to document the lockfile exemption alongside the existing coverage-status definitions.
Reply inline to this comment.
| 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) |
There was a problem hiding this comment.
decodeCoverageStrings truncates an over-long constraint to defaultMaxCoverageConstraintRunes (300) and then appends "..." via truncateRunes, so the value that survives sanitize() can be up to 303 runes — 3 runes past the documented/enforced cap (DefaultFindingsConstraintLimits().MaxRunesPerEntry = 300, which the orchestrator prompt tells reviewers is the hard limit). TestDecodeFindingsConstraintRuneBoundaries only asserts the result ends with "...", not that it respects the rune cap, so this drift isn't caught. Fix by reserving the ellipsis width before truncating, e.g. truncateRunes(value, defaultMaxCoverageConstraintRunes-3), or by post-truncating the whole result (including the suffix) down to defaultMaxCoverageConstraintRunes.
Reply inline to this comment.
… length, docs - ensureSelectedGlobCoverage skips generated lockfiles too, so a reviewer's prompt scope no longer lists a file the accounting layer then exempts; the coverage universe has one owner. Also filter entry.InspectedFiles so scope and coverage rows draw from the same set. - decodeCoverageConstraints (renamed from decodeCoverageStrings): sanitize then clamp, so a truncated/marker-grown entry always fits the 300-rune cap (ellipsis reserved). Test asserts the rune cap, not just the ellipsis. - DefaultFindingsConstraintLimits doc reworded (capped/truncated, not enforced by failure). - docs/checkout-native-review-contract.md documents the lockfile exemption in the readable-files / coverage-status section.
|
Addressed all findings in the latest commit:
|
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 7157adbdc4f5
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| structure:repo-health | 0 |
| harness-engineering:repo-health | 0 |
| architecture:solid | 1 |
go:implementation-tests (1 finding)
Major - internal/pipeline/pipeline.go:1582
This revision adds two new lockfile-exemption branches beyond the one covered by TestBuildReviewerCoverageExemptsGeneratedLockfiles, and neither has a regression test: (1) here in ensureSelectedGlobCoverage, an uncovered lockfile matching a full-scope agent's globs is now deliberately skipped instead of force-assigned — no test drives a changed-file set where a lockfile (e.g. Cargo.lock) matches an agent's FileGlobs, so a future edit that removes or reorders this
continuewould silently start force-assigning lockfiles into agent scope again, which is exactly the coverage regression this PR fixes. (2) in buildReviewerCoverage (pipeline.go:2533),entry.InspectedFiles = filterReviewableFiles(copySortedStrings(result.InspectedFiles))strips a lockfile a reviewer explicitly reported inspecting, but TestBuildReviewerCoverageExemptsGeneratedLockfiles only exercises the skipped_files path, never a result with InspectedFiles containing a lockfile. Add a case to TestEnsureSelectedGlobCoverageAssignsUncoveredMatchingFiles (or a new test) with a lockfile inchangedthat matches an agent's glob, asserting it stays unassigned, and extend the lockfile-coverage test with a result.InspectedFiles entry for a lockfile, asserting it's dropped from the summary's InspectedFiles.
architecture:solid (1 finding)
Nits - internal/llm/contracts.go:373
U-S1: sanitize-before-clamp is the right order (marker.SanitizeModelContent replaces
<!-- codereview:with a 3-rune-longer<!-- codereview:, so the old order could exceed the cap; truncation can only delete a suffix, so it cannot re-form a marker opening). The residue is thatdefaultMaxCoverageConstraintRunes-3encodes a private detail of a helper in another file —truncateRunesappends a literal"..."(adapter.go:401) — so changing that ellipsis to"…"silently makes every caller that budgets for it wrong. The comment documents the coupling rather than removing it. Consider a sibling helper next totruncateRunesthat takes a total budget (e.g.truncateRunesTotal(value, max)returning a string of at mostmaxrunes including the marker), leaving the arithmetic with the code that appends. Defensible as-is given the test at contracts_test.go:213-216 now asserts the ≤300 postcondition.
Reviewer Coverage
go:implementation-tests— complete (constrained); inspected 4 assigned files (5 inspected across reviewers):internal/llm/contracts.go,internal/llm/contracts_test.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go; skipped: none; constraints: nonestructure:repo-health— complete (constrained); inspected 2 assigned files (5 inspected across reviewers):internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go; skipped: none; constraints: Could not run git diff/log between base and head SHAs (git commands required interactive approval unavailable in this session); reviewed the head-checkout source directly instead of the exact unified diff.harness-engineering:repo-health— complete (constrained); inspected 3 assigned files (5 inspected across reviewers):docs/checkout-native-review-contract.md,internal/llm/contracts.go,internal/pipeline/pipeline.go; skipped: none; constraints: nonearchitecture:solid— complete (constrained); inspected 2 assigned files (5 inspected across reviewers):internal/llm/contracts.go,internal/pipeline/pipeline.go; skipped: none; constraints: Reviewed only the two assigned Go files; docs/checkout-native-review-contract.md and the two _test.go files were read as context for intent and test coverage, not reviewed as changed files. Sandbox blocked git in the reviewer checkout, so the diff was reconstructed from head-of-branch files plus the change map rather than read as a patch; changed-line boundaries are inferred. Sandbox blocked running the project's verification (go test ./internal/llm/ ./internal/pipeline/, go vet); findings rest on reading, not on a reproduced build or test run.
Inspected files (5)
docs/checkout-native-review-contract.mdinternal/llm/contracts.gointernal/llm/contracts_test.gointernal/pipeline/pipeline.gointernal/pipeline/pipeline_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 3m 29s | $5.23 | claude-sonnet-5, claude-opus-5 | cr dev
| Field | Value |
|---|---|
| Model | claude-sonnet-5, claude-opus-5 |
| Reviewers | go:implementation-tests, structure:repo-health, harness-engineering:repo-health, architecture:solid |
| Engine | claude_cli · claude-sonnet-5, claude-opus-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 3m 29s wall · 6m 17s compute |
| Cost | $5.23 |
| Tokens | 94 in / 24.8k out |
Per-workstream usage
go:implementation-tests— claude-sonnet-5- In: 24
- Out: 6.1k
- Cache read: 1.1M
- Cache create: 164.3k
- Cost: $1.41
- Duration: 1m 36s
structure:repo-health— claude-sonnet-5- In: 18
- Out: 5.6k
- Cache read: 707.5k
- Cache create: 75.2k
- Cost: $0.75
- Duration: 1m 21s
harness-engineering:repo-health— claude-sonnet-5- In: 16
- Out: 2.6k
- Cache read: 611.7k
- Cache create: 70.0k
- Cost: $0.64
- Duration: 37s
architecture:solid— claude-opus-5- In: 30
- Out: 9.4k
- Cache read: 1.4M
- Cache create: 120.3k
- Cost: $2.15
- Duration: 2m 22s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 1.1k
- Cache read: 113.0k
- Cache create: 37.3k
- Cost: $0.27
- Duration: 19s
| // 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) { |
There was a problem hiding this comment.
This revision adds two new lockfile-exemption branches beyond the one covered by TestBuildReviewerCoverageExemptsGeneratedLockfiles, and neither has a regression test: (1) here in ensureSelectedGlobCoverage, an uncovered lockfile matching a full-scope agent's globs is now deliberately skipped instead of force-assigned — no test drives a changed-file set where a lockfile (e.g. Cargo.lock) matches an agent's FileGlobs, so a future edit that removes or reorders this continue would silently start force-assigning lockfiles into agent scope again, which is exactly the coverage regression this PR fixes. (2) in buildReviewerCoverage (pipeline.go:2533), entry.InspectedFiles = filterReviewableFiles(copySortedStrings(result.InspectedFiles)) strips a lockfile a reviewer explicitly reported inspecting, but TestBuildReviewerCoverageExemptsGeneratedLockfiles only exercises the skipped_files path, never a result with InspectedFiles containing a lockfile. Add a case to TestEnsureSelectedGlobCoverageAssignsUncoveredMatchingFiles (or a new test) with a lockfile in changed that matches an agent's glob, asserting it stays unassigned, and extend the lockfile-coverage test with a result.InspectedFiles entry for a lockfile, asserting it's dropped from the summary's InspectedFiles.
Reply inline to this comment.
| 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) |
There was a problem hiding this comment.
U-S1: sanitize-before-clamp is the right order (marker.SanitizeModelContent replaces <!-- codereview: with a 3-rune-longer <!-- codereview:, so the old order could exceed the cap; truncation can only delete a suffix, so it cannot re-form a marker opening). The residue is that defaultMaxCoverageConstraintRunes-3 encodes a private detail of a helper in another file — truncateRunes appends a literal "..." (adapter.go:401) — so changing that ellipsis to "…" silently makes every caller that budgets for it wrong. The comment documents the coupling rather than removing it. Consider a sibling helper next to truncateRunes that takes a total budget (e.g. truncateRunesTotal(value, max) returning a string of at most max runes including the marker), leaving the arithmetic with the code that appends. Defensible as-is given the test at contracts_test.go:213-216 now asserts the ≤300 postcondition.
Reply inline to this comment.
- TestEnsureSelectedGlobCoverageSkipsLockfiles: a lockfile matching an agent's globs stays unassigned instead of being force-assigned into scope. - Extend the coverage test with a reviewer that reports inspecting Cargo.lock, asserting it's dropped from the summary's InspectedFiles.
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 5fdbcebd8ad0
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| structure:repo-health | 0 |
| harness-engineering:repo-health | 0 |
| architecture:solid | 0 |
Reviewer Coverage
go:implementation-tests— complete (constrained); inspected 4 assigned files (5 inspected across reviewers):internal/llm/contracts.go,internal/llm/contracts_test.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go; skipped: none; constraints: nonestructure:repo-health— complete (constrained); inspected 2 assigned files (5 inspected across reviewers):internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go; skipped: none; constraints: Could not run git diff/log between base and head SHAs (git commands required interactive approval unavailable in this session); reviewed the head-checkout source directly instead of the exact unified diff.harness-engineering:repo-health— complete (constrained); inspected 3 assigned files (5 inspected across reviewers):docs/checkout-native-review-contract.md,internal/llm/contracts.go,internal/pipeline/pipeline.go; skipped: none; constraints: nonearchitecture:solid— complete (constrained); inspected 2 assigned files (5 inspected across reviewers):internal/llm/contracts.go,internal/pipeline/pipeline.go; skipped: none; constraints: Both assigned files are unchanged from the previously reviewed revision (same content at the same lines); the only delta at this head is +31 test lines in internal/pipeline/pipeline_test.go, which was read as context. Reviewed only the two assigned Go files; docs/checkout-native-review-contract.md and the two _test.go files were read for intent and coverage verification, not reviewed as changed files. Sandbox blocked git in the reviewer checkout, so the diff was reconstructed from head-of-branch files plus the change map rather than read as a patch; changed-line boundaries are inferred. Sandbox blocked running the project's verification (go test ./internal/llm/ ./internal/pipeline/, go vet); conclusions rest on reading, not on a reproduced build or test run. The one residual item on these files — defaultMaxCoverageConstraintRunes-3 encoding truncateRunes' literal "..." ellipsis at contracts.go:373 — is already carried by an open inline thread, so it was not re-filed as a duplicate finding.
Inspected files (5)
docs/checkout-native-review-contract.mdinternal/llm/contracts.gointernal/llm/contracts_test.gointernal/pipeline/pipeline.gointernal/pipeline/pipeline_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 2m 04s | $4.52 | claude-sonnet-5, claude-opus-5 | cr dev
| Field | Value |
|---|---|
| Model | claude-sonnet-5, claude-opus-5 |
| Reviewers | go:implementation-tests, structure:repo-health, harness-engineering:repo-health, architecture:solid |
| Engine | claude_cli · claude-sonnet-5, claude-opus-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 2m 04s wall · 3m 14s compute |
| Cost | $4.52 |
| Tokens | 66 in / 11.7k out |
Per-workstream usage
go:implementation-tests— claude-sonnet-5- In: 14
- Out: 2.2k
- Cache read: 777.3k
- Cache create: 111.0k
- Cost: $0.93
- Duration: 42s
structure:repo-health— claude-sonnet-5- In: 14
- Out: 2.4k
- Cache read: 636.5k
- Cache create: 88.9k
- Cost: $0.76
- Duration: 43s
harness-engineering:repo-health— claude-sonnet-5- In: 16
- Out: 2.5k
- Cache read: 706.1k
- Cache create: 83.1k
- Cost: $0.75
- Duration: 34s
architecture:solid— claude-opus-5- In: 16
- Out: 4.2k
- Cache read: 921.3k
- Cache create: 123.1k
- Cost: $1.80
- Duration: 1m 03s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 395
- Cache read: 123.5k
- Cache create: 40.6k
- Cost: $0.29
- Duration: 9s
2 participants
Problem
A reviewer that skips a dependency lockfile (
Cargo.lock,package-lock.json,go.sum, …) is markedincomplete_skipped, andhasIncompleteReviewerCoveragethen downgrades an otherwise-clean APPROVE to a COMMENT. Because a reviewer will always skip a machine-generated lockfile (there's nothing to read line by line), this permanently blocks approval on any PR where a churned lockfile is the only "unreviewed" file — e.g. aCargo.lockregenerated by a dependency-version bump on an otherwise clean PR. Re-running never clears it.Fix
Exempt generated lockfiles from the reviewer coverage universe in
buildReviewerCoverage:changedFilesis filtered before coverage is computed, so an unassigned lockfile can't becomeincomplete_unassigned.scopeis filtered too, so a lockfile explicitly assigned to an agent and skipped can't becomeincomplete_skipped.Lockfiles are reviewed (if at all) through the manifest change that produced them, never line by line, so leaving them out of the coverage obligation is correct — not a loophole.
isGeneratedLockfilematches by basename against the well-known set (Cargo, npm/yarn/pnpm/bun, Go, Bundler, Poetry/Pipenv, Composer, CocoaPods, Nix, Mix).Tests
New
TestBuildReviewerCoverageExemptsGeneratedLockfiles: a reviewer that inspects the real change and skips onlyCargo.lockiscomplete_constrained(notincomplete_skipped), and an unassignedyarn.lockproduces no coverage row.go test ./internal/pipeline/ ./internal/reviewplan/,go vet,gofmtall clean.Found while landing an unrelated PR whose
Cargo.lock(bumped by a dep upgrade) was the only file cr wouldn't sign off on.