Skip to content

fix(web): bound inline comment pagination and stored body size - #7994

Merged
iscekic merged 9 commits into
mainfrom
drain/bound-inline-comment-pagination
Oct 26, 2026
Merged

iscekic merged 9 commits into
mainfrom
drain/bound-inline-comment-pagination

Conversation

@iscekic

@iscekic iscekic commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator

Evidence

Evidence: demonstrated — signed GitHub pull_request deliveries exercised the real Next.js webhook route, PostgreSQL review rows, review preparation, and worker dispatch against a local provider stand-in at commit 2b3a9e08a4; 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:

  • Full-page provider data. The stand-in served 20 full pages of prior inline review data (100 per page) and 12 pages of issue comments for acme/widgets#7.
  • Bounded reads. The app requested inline-comment pages 1–5 only, each with 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.
  • Payload effect. The dispatched review payload carried ## 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.
  • Stored-byte bound. A second delivery whose stored findings each occupied 1.5 KB stopped at the byte budget instead: 4 inline pages read, 350 active in the payload, same 500-character ceiling.
  • Diagnostics. The adapter's log markers reported reachedPageLimit: true, reachedByteLimit: false for the first run and the converse for the second.
  • Database. cloud_agent_code_reviews held a queued row for acme/widgets#7 at the delivered head SHA.
  • CI on this head covers the changed Jest suites.

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.

  1. Question: Bound style: When the page or byte bound is reached, should fetchPRInlineComments return the partial list or throw a controlled error (like findKiloReviewComment does at its scan limit)?
    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
  2. Question: Body cap: Truncate each stored body to what length?
    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)
  3. Question: Byte budget: Cap cumulative stored body bytes at what budget?
    Answer: 64 KB
    Confidence: 0.50 · Precedent: none
  4. Question: Page cap: Reuse MAX_KILO_REVIEW_COMMENT_PAGES (5) for the page cap, or add a separate constant?
    Answer: Reuse 5 pages (Recommended)
    Confidence: 0.80 · Precedent: Least work and no unneeded settings: remove settings that never change (session 1473782d4df9)
  5. Question: Truncation flag: Should the prompt show reviewers that the inline-comment list was truncated (a flag on existingReviewState), or stay silent?
    Answer: No flag (Recommended)
    Confidence: 0.75 · Precedent: Brief asks only for a logged truncation marker. "cut all the unnecessary crap out" (session 61b0bf81a8cc)
  6. Question: Fetch order: Kilobot: with the page cap, oldest-first fetching trims the newest comments (incl. Kilo's own recent inline comments) on PRs with 500+ comments. Fetch newest-first instead?
    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.
  7. Question: Byte budget: Kilobot: the byte budget can't bind with 60-char ASCII bodies. Keep it as defense-in-depth or remove it?
    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.
  8. Question: Marker lookup: The same adapter's existing-comment marker lookup (used to avoid posting a duplicate notice) still follows the whole comment list of a pull request. How should it be hardened?
    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)
  9. Question: Shared scan: Both issue-comment lookups would share one bounded scan helper (newest first). The review-summary lookup already sorts its matches by update time, so ordering does not change its result. Share the helper?
    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"
  10. Question: Scan ends: The bounded issue-comment scan is shared by the review-summary lookup and the marker lookup. Both markers are written once and then maintained in place, so they sit near where they were created (often the oldest end), while bd184b8 made the scan read only the newest pages. Which bounded scan should the shared helper use?
    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.
  11. Question: Prompt render: The stored body is re-truncated in the review prompt: the ## Existing Inline Comments table renders c.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.
  12. Question: Cell content: What should a prompt table row carry once it is wider?
    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.
  13. Question: Byte budget: With 500-char bodies the 64 KB stored-body budget binds after roughly 130 ASCII comments, so the 500-comment page budget is no longer reachable in practice. Keep 64 KB or scale it?
    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.
  14. Question: Untrusted text: The cell now carries ~8x more attacker-controllable text (anyone can comment on a PR). Route it through the existing 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.
  15. Question: Judge recovery: Publication is blocked by repeated judge HTTP 429s. Read-only monitoring confirms throttling of the configured work account; both existing credential references are available. May I temporarily route only this VM's providers.kilo.apiKey to 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"

@iscekic iscekic added drain Work item from the findings drain janitor:trust Janitor finding: trust boundary hardening labels Oct 19, 2026
@kilo-code-bot

kilo-code-bot Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

The only commit since the previous review at 215ed188 adds a paged-author identity test to adapter.fetch-pr-inline-comments.test.ts; the assertions match the current adapter mapping (authoredByKilo from performed_via_github_app.id, position == null for outdated, 500-character body slice, 512 KB byte budget), so no new correctness, security, or memory-lifetime problem is visible.

Assumptions: [INFERENCE] The merge commit 359475d84 brought main's sanitizePromptDataValue and authoredByKilo filtering into the branch, so the PR's remaining prompt delta is only the shared MAX_INLINE_COMMENT_BODY_CHARS slice; the PR description's Proxy answers and Evidence section still narrate 60-character bodies, a 64 KB budget, and a 384-comment multi-byte stop, which no longer match the code and should be treated as stale. Two earlier bot inline comments on the byte budget and the truncated-list behavior remain answered by the author but are not re-raised because the chosen design (partial list with a logged truncation flag) matches the recorded decision. No tests or typecheck were executed in this read-only review.

Files Reviewed (7 files)
  • apps/web/src/lib/code-reviews/core/constants.ts
  • apps/web/src/lib/code-reviews/prompts/generate-prompt.ts
  • apps/web/src/lib/code-reviews/prompts/generate-prompt.test.ts
  • apps/web/src/lib/integrations/platforms/github/adapter.ts
  • apps/web/src/lib/integrations/platforms/github/adapter.fetch-pr-inline-comments.test.ts
  • apps/web/src/lib/integrations/platforms/github/adapter.find-kilo-review-comment.test.ts
  • apps/web/src/lib/integrations/platforms/github/adapter.has-pr-comment-with-marker.test.ts
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 Summary

The 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: [INFERENCE] The stored-body byte budget is 512 KB in code while the PR description's proxy answers and Evidence section still describe 60-character bodies, a 64 KB budget, and a 384-comment multi-byte stop from 7d5a1a84ac; those sections are stale and the owner should treat the code as authoritative. Inline comment bodies are user-controlled and now contribute up to 500 characters per row (20 rows) to the review prompt, sanitized exactly as before; this widens an already-existing prompt-injection surface rather than creating a new one. No test execution or typecheck was performed in this read-only review.

Files Reviewed (5 files)
  • apps/web/src/lib/code-reviews/core/constants.ts
  • apps/web/src/lib/code-reviews/prompts/generate-prompt.ts
  • apps/web/src/lib/code-reviews/prompts/generate-prompt.test.ts
  • apps/web/src/lib/integrations/platforms/github/adapter.ts
  • apps/web/src/lib/integrations/platforms/github/adapter.fetch-pr-inline-comments.test.ts

Previous review (commit 7d5a1a8)

Status: No Issues Found | Recommendation: Merge

Executive Summary

The 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: [INFERENCE] GitHub returns issue comments oldest-first and issues.listComments ignores order parameters, as stated in the PR description; the scan still cannot cover middle pages, so a `` summary created after roughly 300 pre-existing comments on a PR with more than 500 comments would land in a skipped page and findKiloReviewComment would throw (handled by callers as unknown state). Comment ordering during pagination could also shift pages if new comments arrive mid-scan. No test execution was performed in this read-only review.

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 0
Files Reviewed (3 files)
  • apps/web/src/lib/integrations/platforms/github/adapter.ts
  • apps/web/src/lib/integrations/platforms/github/adapter.find-kilo-review-comment.test.ts
  • apps/web/src/lib/integrations/platforms/github/adapter.has-pr-comment-with-marker.test.ts

Previous review (commit bd184b8)

Status: 1 Issue Found | Recommendation: Address before merge

Executive Summary

The 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: [INFERENCE] GitHub returns issue comments oldest-first and issues.listComments ignores ordering parameters (as stated in the PR description); if a Link header omits rel="last", the helper falls back to page 1 only, which could also miss later comments.

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0
Issue Details (click to expand)

WARNING

File Line Issue
apps/web/src/lib/integrations/platforms/github/adapter.ts 633 Reading only the newest five issue-comment pages misses the review-summary marker on PRs with more than 500 comments; findKiloReviewComment then throws, losing existing state in review preparation and aborting the completion-path summary update.
Files Reviewed (3 files)
  • apps/web/src/lib/integrations/platforms/github/adapter.ts - 1 issue
  • apps/web/src/lib/integrations/platforms/github/adapter.has-pr-comment-with-marker.test.ts
  • apps/web/src/lib/integrations/platforms/github/adapter.find-kilo-review-comment.test.ts

Fix these issues in Kilo Cloud

Previous review (commit 67e0892)

Status: No Issues Found | Recommendation: Merge

Executive Summary

The incremental change routes hasPRCommentWithMarker through the shared bounded, newest-first issue-comment scan and adds focused tests for the app-attribution rule and page cap; the pagination bookkeeping matches the adapter's existing scan semantics and no new defects were found in the changed lines.

Assumptions: [INFERENCE] GitHub's issues.listComments honors sort/direction (already relied on by apps/web/src/lib/bot/platforms/github.ts:213); if it were ignored, a bounded marker scan could miss an older billing notice and post a duplicate.

Files Reviewed (3 files)
  • apps/web/src/lib/integrations/platforms/github/adapter.ts
  • apps/web/src/lib/integrations/platforms/github/adapter.has-pr-comment-with-marker.test.ts
  • apps/web/src/lib/integrations/platforms/github/adapter.find-kilo-review-comment.test.ts

Previous review (commit 0de04da)

Status: No Issues Found | Recommendation: Merge

Executive Summary

The 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)
  • apps/web/src/lib/integrations/platforms/github/adapter.fetch-pr-inline-comments.test.ts

Notes

  • The new test at line 151 is internally consistent with fetchPRInlineComments: 60 CJK characters are 180 UTF-8 bytes, ceil(65536 / 180) = 365, and 365 bodies span 4 pages, so 3 assertions (length, page count < MAX_PAGES, byte total >= 64 KB) all hold.
  • No new issues were found in the added lines; the test intentionally documents that the byte budget is a soft cap, since the body that crosses the threshold is retained.

Previous review (commit 317eff3)

Status: No Issues Found | Recommendation: Merge

Executive Summary

The incremental change adds sort: 'created', direction: 'desc' to fetchPRInlineComments, so a page or byte cap now retains the newest review comments for duplicate detection; the change is correct and high confidence, and both prior findings are resolved.

Files Reviewed (1 file)
  • apps/web/src/lib/integrations/platforms/github/adapter.ts

Notes on prior findings

  • Previous WARNING (truncated list drops the newest comments, risking duplicate posts): resolved. With direction: 'desc' the first pages hold the newest comments, and the prompt's "Existing Inline Comments" table (active.slice(0, 20)) now reflects the most recent active comments rather than the oldest.
  • Previous SUGGESTION (64 KB byte budget cannot bind): not reproducible. body.slice(0, 60) counts UTF-16 units, so 60 CJK characters are 180 UTF-8 bytes and 500 such comments exceed 64 KB, making reachedByteLimit reachable for non-ASCII content.

No new issues were found in the added lines.

Previous review (commit 5d9d901)

Status: 2 Issues Found | Recommendation: Address before merge

Executive Summary

Bounding 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

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 1
Issue Details (click to expand)

WARNING

File Line Issue
apps/web/src/lib/integrations/platforms/github/adapter.ts 759 The partial list is used with no error signal. Unlike the sibling findKiloReviewComment, which throws at its scan limit, the newest comments (including Kilo's own prior inline comments) can fall outside the 500-comment window and go undetected, risking duplicate comments being posted.

SUGGESTION

File Line Issue
apps/web/src/lib/integrations/platforms/github/adapter.ts 753 The 64 KB byte budget cannot bind: bodies are sliced to 60 chars before counting and at most 500 comments are stored, so the page cap always triggers first and reachedByteLimit is effectively dead code.
Files Reviewed (2 files)
  • apps/web/src/lib/integrations/platforms/github/adapter.ts - 2 issues
  • apps/web/src/lib/integrations/platforms/github/adapter.fetch-pr-inline-comments.test.ts - 0 issues

Fix these issues in Kilo Cloud


Reviewed by deepseek-flash-latest · Input: 76.4K · Output: 9.9K · Cached: 647.8K

Review guidance: REVIEW.md from base branch main

@iscekic iscekic added the merge:human Substantive functional risk requires human review. label Oct 19, 2026
@iscekic
iscekic enabled auto-merge October 19, 2026 17:46
@iscekic
iscekic disabled auto-merge October 20, 2026 19:28
@iscekic iscekic added merge:blocked merge:human Substantive functional risk requires human review. merge:blocked:insufficient-e2e Verification does not cover the change and removed merge:human Substantive functional risk requires human review. merge:blocked labels Oct 21, 2026
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.
@iscekic iscekic removed merge:human Substantive functional risk requires human review. merge:blocked:insufficient-e2e Verification does not cover the change labels Oct 23, 2026
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.
@iscekic iscekic added merge:blocked:insufficient-e2e Verification does not cover the change merge:blocked:kilobot-not-ready Review bot has not recommended merge at this head merge:bot Bot-owned review; merge when ready once readiness passes and a review approves. labels Oct 23, 2026
…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.
@iscekic iscekic removed merge:blocked:kilobot-not-ready Review bot has not recommended merge at this head merge:bot Bot-owned review; merge when ready once readiness passes and a review approves. merge:blocked:insufficient-e2e Verification does not cover the change labels Oct 23, 2026
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.
@iscekic iscekic removed merge:bot Bot-owned review; merge when ready once readiness passes and a review approves. merge:blocked:insufficient-e2e Verification does not cover the change labels Oct 23, 2026
@iscekic

iscekic commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator Author

bot: Done in 215ed18. Existing inline review comments now keep up to 500 characters, through one MAX_INLINE_COMMENT_BODY_CHARS constant that the GitHub adapter and the duplicate-detection table in the review prompt both use, so a row shows the whole finding title instead of a fragment. The stored-body byte budget is 512 KB, which keeps the five-page, 100-per-page lookup budget reachable at that per-comment size, and the cell text passes the existing prompt sanitizer before the markdown table escape. The app-author check in hasPRCommentWithMarker, the page bound, and the byte cap are unchanged.

Verified on this VM at that commit: a signed pull_request webhook ran through the real app to the review-preparation path, and the captured prompt payload carried the full 160-character finding title and a 499-character row, each on one line. The lookup read one full page plus one short page (100 comments) with no bound reached, and both affected jest suites pass (62 tests).

@iscekic iscekic added merge:blocked:insufficient-e2e Verification does not cover the change merge:bot Bot-owned review; merge when ready once readiness passes and a review approves. labels Oct 23, 2026
@iscekic iscekic added merge:blocked:insufficient-e2e Verification does not cover the change merge:bot Bot-owned review; merge when ready once readiness passes and a review approves. merge:human Substantive functional risk requires human review. and removed merge:bot Bot-owned review; merge when ready once readiness passes and a review approves. merge:blocked:insufficient-e2e Verification does not cover the change merge:human Substantive functional risk requires human review. labels Oct 23, 2026
@iscekic
iscekic enabled auto-merge October 24, 2026 14:37

@eshurakov eshurakov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shallow review: no critical issues found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

drain Work item from the findings drain janitor:trust Janitor finding: trust boundary hardening merge:bot Bot-owned review; merge when ready once readiness passes and a review approves.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants