Repository navigation
fix(pr-review): one visible review per pull request, and a confidence that holds a known bug below safe - #1973
Merged
Merged
Conversation
… that holds a known bug below safe Measured on the 201 reviews Maple posted on #1915 to #1969: - Each push posted a new summary and none were folded, so #1915 carried 20 of them. Once a review's summary is posted, the App's earlier ones are minimized as outdated (GraphQL minimizeComment, best effort). What was checked, the coverage table, production traffic and the telemetry inventory move to the check run; the comment points there. Open errors group repeated titles. - 59 reviews read 7 or more ("likely safe to merge") with a correctness, security or performance warning open. Such a warning now holds confidence at 6, a security warning at 4. - A 10 was followed by a new defect on the next push 14% of the time. A pull request whose earlier reviews found defects that are now fixed is held at 8: a re-review reads only what changed. - Finding bodies ran a median of 74 words and restated the fix printed under them. record_finding and submit_review refuse a body over 60 words or a suggestion over 35 (bounded retries), and the prompt gets a severity rubric with examples from these reviews. review-signal.corpus.test.ts is the check: it reads every published summary back into its report (the recomputed factors line matches all 201), renders and scores it with the current code, and holds bars on the visible text (36% of before), on a safe number beside an open defect (59 to 0), on five heads labelled as misses (all 10 before, 8 now) and on the top band (next-push defects after a 9 or 10: 14% to 4%). Refresh with `bun run --cwd apps/ai review:signal <owner/repo> <first> <last>`.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 23 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Maple review🟡 Confidence 6/10 · needs attention Folds earlier review summaries away with GitHub
Before merge
Findings🟠 Warning · F1 · The close-out exemption of the new submit-review length refusal (`partialtests · 🟠 Warning · F2 · The new
|
| File | Calls/day | Busiest operations |
|---|---|---|
packages/backend/src/services/integrations/vcs/vendor/github/GithubAppClient.ts |
112 | GithubAppClient.listReviewComments 82/day, 0.0% errPOST /graphql 15/day, 0.0% errPOST /api/graphql 15/day, 0.0% err |
Telemetry this change adds and removes (4)
- ➕ attribute
maple.pr_review.overlong_findings·apps/ai/src/chat/tools.ts:507 - ➕ attribute
maple.pr_review.record_outcome·apps/ai/src/chat/tools.ts:590 - ➕ span name
GithubAppClient.minimizeEarlierComments·packages/backend/src/services/integrations/vcs/vendor/github/GithubAppClient.ts:1461 - ➕ attribute
maple.pr_review.earlier_comments_hidden·packages/backend/src/services/pr-review/PrReviewService.ts:2893
What was checked
Arr.chunksOfkeepsnodes(ids:)under GitHub's 100-id limit, andisMinimized === falseskips comments already hiddenpriorDefectscounts only resolved or this-round-fixed defects;dismissedfindings are excluded (PrReviewService.ts:2626)- The hide step cannot lose the review: it runs after publish and its failure is a logged warning (
PrReviewService.ts:2873–2893)
Observability coverage: 1 of 1 changes observable
| Change | Kind | Observable | Evidence |
|---|---|---|---|
| GithubAppClient.minimizeEarlierComments (issue-comment list, comment states, minimizeComment mutations) | outbound call | yes | Effect.fn("GithubAppClient.minimizeEarlierComments") span; the caller records maple.pr_review.earlier_comments_hidden and logs a warning on refusal (PrReviewService.ts:2880) |
Files not reviewed (48)
The review ended before it read these diffs, so nothing above vouches for them.
apps/ai/package.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/labels.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1916.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1918.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1919.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1920.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1922.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1923.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1924.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1925.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1926.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1927.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1928.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1929.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1930.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1932.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1933.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1934.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1935.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1936.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1937.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1938.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1940.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1941.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1943.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1944.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1945.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1946.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1947.jsonpackages/backend/src/services/pr-review/__fixtures__/review-signal/maple-1948.json- and 18 more
af21af7 · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.
![maple-review-bot[bot]](https://avatars.githubusercontent.com/in/4108169?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL read visibleText's single-pass tag and comment removal as an incomplete HTML sanitizer. It only counts characters for the eval, but repeating each removal to a fixpoint is what the measure means anyway.
Maple review🟡 Confidence 6/10 · needs attention The head commit keeps the corpus text measure repeating each markup strip until the text stops changing; I re-ran it over all 280 fixture comments and every count is identical. The one file changed since the last round is clean, but the three test-gap warnings from that round are still open.
Before merge
Still open from earlier reviews
Production impactProduction traffic of the changed files (last 7 days)
Telemetry this change adds and removes (4)
What was checked
|
…e close-out length exemption Addresses F1 to F3 of the review: open errors with one title fold into a summed row and are no longer expanded, the comment keeps only breaks and open errors while the check keeps traffic, the telemetry inventory and log costs, and a close-out is never refused for length.
Maple review🟢 Confidence 10/10 · safe to merge Third commit adds the tests the earlier rounds asked for: the length-refusal cap, the close-out exemption and the once-only submit refusal, plus issue grouping and the brief comment section. Test-only, no production code touched. Fixed since the last review
Production impactProduction traffic of the changed files (last 7 days)
Telemetry this change adds and removes (4)
What was checked
|
Maple: after this shipped✅ Shipped clean. Nothing this change touches got worse in the hour after it deployed. Deployed to
Maple looks once, an hour after the first deploy that carries the merge. |
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
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.
Why
The review bot's comments were tiring to read, and its confidence number was off. I measured both on the 201 reviews it posted on #1915 to #1969:
applyJson, perf(ai): run_sql seeks trace-id lookups through trace_index and stops at the row cap #1960limitedWithinCapand three heads of fix(ai): run_sql reruns misspelt column names and keeps internal SQL out of errors #1959replaceIdentifier; they are labelled inlabels.json.What changes
hideEarlierComments→ GraphQLminimizeComment). This is best effort: a refusal is logged and the review still posts.record_findingrefuses a body over 60 words or a suggestion over 35, up to 4 times per pass.submit_reviewrefuses once. A close-out is never refused. The prompt gains a severity rubric with examples taken from these reviews, and the fix now goes only insuggestionorreplacement.Verification
review-signal.corpus.test.tssnapshots every review summary and inline note from 47 real PRs (__fixtures__/review-signal/). It refreshes withbun run --cwd apps/ai review:signal MapleTechLabs/maple <first> <last>.The test reads each summary back into its report. For all 201 reviews, the recomputed score breakdown matches the published one. It then renders and scores each review with the current code and holds these bars:
Mutation check: putting back the old defect cap fails the "never reads safe with a defect open" bar. Putting back the old cap and turning off history fails three bars.
Typecheck passes in
domain,backendandai. These suites pass: domainpr-review(37 tests), backendpr-review+vcs(500), aichat(292). Lint is clean on the changed files.Not verified
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.