feat: inline review comments on specific lines #26
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: rodin/review-bot#26
Reference in New Issue
Block a user
Delete Branch "feat/inline-review-comments"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Findings that reference a file+line within the diff are now posted as inline comments directly on that line. Findings outside the diff range stay in the body only.
Changes
gitea/diff.go: Parse unified diff to extract new-file line numbers per hunkgitea/client.go:PostReviewaccepts optional[]ReviewCommentcmd/review-bot/main.go: Maps findings → inline comments when line is in diffTests
Closes #26
- Hunk headers without comma ("@@ -1 +1 @@") now parse correctly by splitting on comma OR space instead of comma only - Explicit skip for "\ No newline at end of file" lines (was already safe but now documents intent) - Tests added for both edge cases (TDD: tests written first, confirmed failure, then fixed) Addresses sonnet findings #1 and #2 from PR #26 review.Gpt Review
Summary
The changes add support for mapping findings to inline comments on changed lines in the diff, refactoring review update logic, and extending test coverage for new behaviors. Implementation is idiomatic, test-backed, and aligns with repo conventions.
Recommendation
APPROVE — The PR is well-structured and incorporates a robust solution for associating findings with inline comments, including utility for diff range mapping and a conservative in-place review update strategy. The changes are covered with comprehensive tests and maintain clean separation of concerns. No issues found—ready to merge.
Review by gpt
Security Review
Summary
The changes add inline review comments mapped to diff lines and introduce safe, well-structured Gitea client methods for posting comments and editing review bodies. CI passed and the code follows good error handling and path safety practices, with only minor hardening opportunities identified.
Findings
gitea/client.gocmd/review-bot/main.goRecommendation
APPROVE — Overall, the implementation is sound and security-conscious (notably the workspace-bound file reads and consistent error wrapping). To further harden:
With these minor adjustments, the feature should be secure in typical deployments.
Review by security
Sonnet Review
Summary
Well-scoped feature addition enabling inline review comments with a solid diff parser and comprehensive tests. API changes are propagated correctly and review update strategy is improved to preserve threads.
Findings
gitea/client.gocmd/review-bot/main.gogitea/diff.gocmd/review-bot/main.goRecommendation
APPROVE — The implementation cleanly introduces inline review comments by parsing unified diffs and extending the Gitea client to submit comments alongside reviews. Tests cover the diff parser (multi-hunk, new files, edge markers) and the new client behavior (comments omitempty, EditComment, timeline lookup). The refactored review update strategy (edit-in-place, supersede on state change, worst-wins escalation without deletion) is thoughtful and documented in the new REVIEW_STRATEGY.md. One point to double-check is the payload schema for inline comments: "new_position" may not match what Gitea expects; confirm against the Gitea API (whether it requires "position" or "line" semantics, and whether values are diff positions vs. file line numbers). Aside from that, the changes are solid and CI has passed. I recommend approval.
Review by sonnet
@@ -452,0 +482,4 @@for _, r := range reviews {if r.Stale {continue}[MINOR] old findingSuperseded by newer review@@ -0,0 +43,4 @@currentFile = ""continue}[NIT] Hunk header parsing uses
strings.Split(line, "+")and then trims at comma/space. While adequate for typical diffs, consider a more targeted parse (e.g., locating the first '+' between the '@@' markers) to be more robust against unusual hunk headers.Replace the delete-and-repost strategy with edit-in-place: 1. No existing review → POST new (first run) 2. Same state, same body → skip entirely (threads preserved) 3. Same state, body changed → PATCH body in place via timeline API 4. State change needed → PATCH old body to "Superseded", POST new This preserves conversation threads on inline comments. Replies to findings are never lost. The only time a new review is posted is on first run or when the state transitions (APPROVED ↔ REQUEST_CHANGES). New Gitea client methods: - EditComment: PATCH /repos/{owner}/{repo}/issues/comments/{id} - GetTimelineReviewCommentID: finds the comment ID for a review body by scanning the issue timeline for the sentinel Also simplifies shouldEscalate: removes the login parameter requirement for pre-posting scenarios (uses findOwnReview to get login from existing review instead). Tests: findOwnReview (4 cases), EditComment (2 cases), GetTimelineReviewCommentID (2 cases), shouldEscalate (8 cases updated).Sonnet Review
Summary
Solid feature addition: parses unified diffs and posts inline comments with good test coverage. However, the review escalation logic was altered in a way that can incorrectly post APPROVED when a sibling bot review has REQUEST_CHANGES on the first run, which is a functional regression.
Findings
cmd/review-bot/main.gogitea/client.gocmd/review-bot/main.goexistingEvent == r.Stateis redundant since existingEvent is assigned from r.State. Simplify tor.State == newEvent && r.Body == newBody.Recommendation
REQUEST_CHANGES — Address the escalation logic regression to ensure the bot doesn't post APPROVED when a sibling review from the same user (same token) has REQUEST_CHANGES on a first run without a prior sentinel. Options:
Additionally, in gitea/client.go’s EditComment, handle the json.Marshal error instead of ignoring it to comply with the repository’s error-handling conventions. Finally, clean up the redundant comparison in reviewUnchanged for clarity. With these changes, the inline comment feature and review update strategy will be robust and aligned with the intended behavior.
Review by sonnet
@@ -254,3 +265,4 @@existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber)if err != nil {log.Printf("Warning: could not list existing reviews: %v", err)} else {[MAJOR] Escalation now happens before posting and depends on deriving ownLogin from an existing review containing the sentinel. On the first run (no existing review with this sentinel), ownLogin is empty and escalation is skipped, so an APPROVED review may be posted even if a sibling bot (same token) already has REQUEST_CHANGES. Previously this was handled by escalating after posting using the posted user login.
@@ -449,3 +496,4 @@}return false}[NIT] In reviewUnchanged, the condition
existingEvent == r.Stateis redundant since existingEvent is assigned from r.State. Simplify tor.State == newEvent && r.Body == newBody.@@ -346,0 +385,4 @@return 0, fmt.Errorf("parse timeline (page %d): %w", page, err)}for _, ev := range events {if ev.Type == "review" && strings.Contains(ev.Body, sentinel) {[MINOR] EditComment marshals the payload with json.Marshal but ignores the error (data, _ := json.Marshal(payload)). This can mask unexpected failures; handle and return the marshal error similar to PostReview.
Security Review
Summary
Security-focused changes look sound: inline comments are restricted to lines present in the diff, API requests use context and timeouts, and edit/escalation logic avoids destructive actions when not permitted. CI passed. I have a couple of minor hardening suggestions.
Findings
gitea/client.gocmd/review-bot/main.goRecommendation
APPROVE — Overall, the implementation is secure and aligns with best practices: inline comments are only posted on lines verified from the parsed diff, all HTTP calls use a bounded http.Client with context, and update-in-place logic gracefully falls back on permission errors. To further harden the code: (1) use url.PathEscape for owner and repo in PostReview (and other endpoints that still use raw values) to prevent path mishandling; (2) tighten sibling review identification by cross-checking the bot's own login or another reliable identifier to prevent sentinel spoofing from influencing escalation decisions. With these minor adjustments, the changes should be robust.
Review by security
@@ -438,14 +477,52 @@ func validateReviewerName(name string) error {return nil[MINOR] shouldEscalate relies on detecting a 'sentinel' substring in review bodies to identify sibling bot reviews. An untrusted user could spoof the sentinel in their own review, potentially causing unintended escalation. Consider additionally verifying the matched review's user login matches the bot's account (derived independently from the API) before treating it as a sibling.
@@ -131,15 +138,18 @@ func (c *Client) GetFileContentRef(ctx context.Context, owner, repo, filepath, r// PostReview submits a review to a PR and returns the created review.// event should be "APPROVED" or "REQUEST_CHANGES".[MINOR] PostReview constructs the reviews API URL using owner and repo without url.PathEscape, which can lead to malformed paths if these contain unexpected characters or slashes. While inputs are typically controlled, escaping path segments (as done elsewhere in this client) is a defense-in-depth improvement.
Ship it.