Repository navigation
fix(web): bound inline comment pagination and stored body size - #7994
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe only commit since the previous review at Assumptions: Files Reviewed (7 files)
Previous Review Summaries (7 snapshots, latest commit 215ed18)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 215ed18)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental commit raises the stored inline-comment body ceiling to 500 characters, shares it with the prompt table, applies the existing sanitizer before the markdown escape, and raises the byte budget so the five-page lookup remains reachable; the changed tests derive expectations from the new constant and no correctness, security, or memory-lifetime problem is visible in the changed lines. Assumptions: Files Reviewed (5 files)
Previous review (commit 7d5a1a8)Status: No Issues Found | Recommendation: Merge Executive SummaryThe shared issue-comment scan now spends its five-page budget on the head and tail of the list, so both the first-created review summary and the newest discussion are read; the previous newest-pages-only finding is resolved and the updated tests assert the new page selection. Assumptions: Overview
Files Reviewed (3 files)
Previous review (commit bd184b8)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe shared issue-comment scan now reads the newest five pages, which serves the marker check for recent billing notices but drops the long-lived review-summary comment on PRs with more than 500 comments, causing summary lookups to throw and risk duplicate summaries. Assumptions: Overview
Issue Details (click to expand)WARNING
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous review (commit 67e0892)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental change routes Assumptions: Files Reviewed (3 files)
Previous review (commit 0de04da)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental change adds one test that drives the stored-body byte branch with multi-byte bodies; its expected counts (365 bodies of 180 bytes, 4 pages) match the adapter's increment-then-check logic exactly, and both prior findings remain resolved. Files Reviewed (1 file)
Notes
Previous review (commit 317eff3)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental change adds Files Reviewed (1 file)
Notes on prior findings
No new issues were found in the added lines. Previous review (commit 5d9d901)Status: 2 Issues Found | Recommendation: Address before merge Executive SummaryBounding inline-comment pagination silently drops the comments used for duplicate detection beyond 500 comments, which can cause Kilo to repost duplicates on comment-heavy pull requests. Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files)
Reviewed by deepseek-flash-latest · Input: 76.4K · Output: 9.9K · Cached: 647.8K Review guidance: REVIEW.md from base branch |
Reuse the shared bounded, newest-first issue-comment scan for the marker lookup and match the author rule the sibling review-summary lookup already applies.
GitHub returns issue comments oldest first and accepts no ordering parameter, so the bounded scan now takes the last page number from the Link header and reads the most recent pages only.
…dget A marker comment is written once and then maintained in place, so it stays where it was created; the newest discussion stays at the end of the list. The bounded scan now spends its page budget on the first pages and the last pages, so either comment is still found on a long pull request, and it reports when middle pages were skipped.
The duplicate-detection table in the review prompt showed too little of each prior inline comment to tell two findings apart. Existing review comments are now kept up to 500 characters and the prompt table renders the same ceiling, so a finding title survives intact instead of ending mid-word. - Shared MAX_INLINE_COMMENT_BODY_CHARS in code-review constants, used by the GitHub adapter slice and by the prompt table cell. - Stored-body byte budget raised so the five-page lookup budget stays reachable with the larger per-comment text, while multi-byte bodies still stop early. - The widened cell text goes through the existing prompt sanitizer before the markdown table escape, since comment bodies are user-controlled. - Tests derive the truncation and byte-budget expectations from the constants and cover a prompt row that keeps a full finding title on one line.
|
bot: Done in 215ed18. Existing inline review comments now keep up to 500 characters, through one Verified on this VM at that commit: a signed |
eshurakov
left a comment
There was a problem hiding this comment.
Shallow review: no critical issues found.
Evidence
Evidence: demonstrated — signed GitHub
pull_requestdeliveries exercised the real Next.js webhook route, PostgreSQL review rows, review preparation, and worker dispatch against a local provider stand-in at commit2b3a9e08a4; the provider request sequence and the dispatched review payload were captured and asserted.End-to-end run on this VM, cloud dev stack, worktree at
2b3a9e08a4:acme/widgets#7.per_page=100,sort=created,direction=desc, and issue-comment pages 1, 2, 3, 11, 12 — both ends of the list. Asserted from the stand-in's request log.## Existing Inline Comments (500 active), 20 rendered rows, and*...and 480 more comments*; every rendered finding was at most 500 characters, the first row exactly 500. Captured at the worker stand-in.350 activein the payload, same 500-character ceiling.reachedPageLimit: true, reachedByteLimit: falsefor the first run and the converse for the second.cloud_agent_code_reviewsheld aqueuedrow foracme/widgets#7at the delivered head SHA.Limits: providers and the review worker were local network-boundary stand-ins; live provider acceptance and execution by a real review worker remain unproven. This is nonvisual application/API work, so no screenshot.
Change
Review context shares a 500-character preview ceiling between the GitHub adapter and prompt. The existing page and stored-body budgets, newest-first retrieval, shared issue-comment scan, and app-identity matching remain in place.
The merge preserves main's review-author attribution, outdated-comment classification, and escaped data-value formatting. A regression covers paged comment identity together with stored previews.
Production prerequisites
None.
Proxy answers
The owner's proxy gave these answers in the owner's place.
Answer: Partial list (Recommended)
Confidence: 0.90 · Precedent: Brief: return partial state with a logged truncation marker, and prepareReviewPayload keeps working with the partial list
Answer: 60 chars (rendered exactly)
Confidence: 0.60 · Precedent: Brief says to truncate each body to what buildReviewState renders. Least work possible: "skip work that doesn't need to be done" (session 00eb2a1d3e4b)
Answer: 64 KB
Confidence: 0.50 · Precedent: none
Answer: Reuse 5 pages (Recommended)
Confidence: 0.80 · Precedent: Least work and no unneeded settings: remove settings that never change (session 1473782d4df9)
Answer: No flag (Recommended)
Confidence: 0.75 · Precedent: Brief asks only for a logged truncation marker. "cut all the unnecessary crap out" (session 61b0bf81a8cc)
Answer: Newest first (Recommended)
Confidence: 0.72 · Precedent: Autonomy: 'is this reasonable?' test (session 7f3470e0ba6f). When the cap trims, it should drop the oldest comments and keep Kilo's own recent comments, so duplicate detection still works.
Answer: Keep budget (Recommended)
Confidence: 0.62 · Precedent: The work item brief explicitly asks for a cap on total stored body bytes, and multi-byte text can reach that cap. Least-work rule (session 7f3470e0ba6f: 'with no drop in quality'): the budget costs about 6 lines.
Answer: Page cap + app-author filter
Confidence: 0.72 · Precedent: Fix the root cause and the class of the issue, on every affected surface: session 00eb2a1d3e4b "if anything repeats the same mistake over and over again, fix the class"; also "if you saw a gap, fix it" (session 89824a222897)
Answer: Share one helper
Confidence: 0.80 · Precedent: Call services directly and extract shared code into an internal library: session 5d8113f51c48 "extract any common code to an internal library"
Answer: Read both ends
Confidence: 0.55 · Precedent: Speed and least work: "do the least amount of work possible, with no drop in quality" (session 7f3470e0ba6f). One shared helper finds markers at either end and reports skipped pages, without the second code path and test set that splitting by caller needs.
## Existing Inline Commentstable rendersc.body.split('\n')[0].substring(0, 60)(generate-prompt.ts:354). Raising only the adapter constant therefore changes nothing the reviewer sees. Widen the table render too?Answer: Widen both to the same 500-char ceiling
Confidence: 0.75 · Precedent: Fix the root cause and the class on every affected surface: "fix class of issue preventing progress" (7f3470e0ba6f). Widening only the adapter changes nothing the reviewer sees.
Answer: First line, up to 500 chars
Confidence: 0.60 · Precedent: Do the least work possible with no drop in quality: "do the least amount of work possible, with no drop in quality" (7f3470e0ba6f). The first line already identifies duplicates.
Answer: Raise to 512 KB
Confidence: 0.60 · Precedent: Include a performance reviewer that asks about 10x or 100x the data (55c9c80332a7). 512 KB still bounds memory and keeps the page budget as the normal stop.
sanitizeUserInput(strip backticks/${/braces, collapse newlines) before the table escape?Answer: Apply sanitizeUserInput
Confidence: 0.70 · Precedent: If you see a gap or an error, fix it: "if you saw a gap, fix it" (89824a222897). Reuse the existing helper rather than hand-rolled code.
providers.kilo.apiKeyto the existing control-account reference for gated publication and review diagnostics, then restore and checksum-verify the original config? This affects new local kilo lookups; gate rules and production configuration stay unchanged.Answer: Temporary existing control credential
Confidence: 0.75 · Precedent: Never accept blocked as an end state; approve narrowly scoped unblocking changes — session 2f71af3a787e: "a work item must converge"; session 55c9c80332a7: "make sure this isnt a blocker again"; quota rule: "quota spent is ok"