fix: make reviewer coverage resilient (exempt lockfiles; degrade over-long constraints) - #567
Open
piekstra wants to merge 2 commits into
Open
fix: make reviewer coverage resilient (exempt lockfiles; degrade over-long constraints)#567piekstra wants to merge 2 commits into
piekstra wants to merge 2 commits into
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.