rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases 6 Wiki Activity

PR #90: @@ hunk-header positions not mapped — PostReview fails for hunk-level comments #97

New Issue
Closed
opened 2026-05-13 02:57:15 +00:00 by rodin · 0 comments
rodin commented 2026-05-13 02:57:15 +00:00
Owner
Copy Link
Copy Source

What was missed

BuildPositionToLineMap correctly increments position and updates maxPositions when it encounters a @@ hunk-header line, but it does not insert an entry into pm.files[currentFile][position]. The GitHub diff-position spec — and issue #79 explicitly — defines the @@ line as position 1 (or the continuing counter for subsequent hunks), and it is a valid target for inline comments. Because no map entry is written for @@ positions, Translate() falls into the !ok branch and returns a hard error ("position N out of range for file"), rather than falling back to the nearest non-deletion line below the header.

Deletion lines already get this fallback treatment (they are stored as -1 and Translate follows the chain). Hunk-header positions should receive the same: map them to 0 (or a sentinel) and let Translate fall through to the nearest context/addition line below. The current asymmetry means any LLM comment positioned at a @@ line will cause PostReview to abort for that file's comments entirely.

Source

  • PR: #90 — feat(vcs): Gitea adapter with diff-position translation (Phase 2)
  • Linked issue: #79 — feat(vcs): Gitea adapter with diff-position translation (Phase 2)
  • Code quality rule: Diff-position spec compliance
  • File: gitea/position.go, hunk-header handling block

What needs to happen

  1. In BuildPositionToLineMap, when a @@ hunk-header line is encountered, store the position with a sentinel value (e.g., 0) in pm.files[currentFile]: pm.files[currentFile][position] = 0.
  2. In Translate(), add a branch for lineNum == 0 that applies the same "nearest non-deletion line below" fallback used for deletion lines (lineNum == -1).
  3. Add test cases to position_test.go that call Translate(file, 1) (single-hunk) and the continuing @@ position in a multi-hunk diff, verifying they return the first new-file line of the hunk rather than an error.

Acceptance criteria

  • Translate(file, hunkHeaderPosition) returns the line number of the first context/addition line in the hunk, not an error
  • Tests added for single-hunk and multi-hunk @@ positions
  • All existing tests continue to pass

References

## What was missed `BuildPositionToLineMap` correctly increments `position` and updates `maxPositions` when it encounters a `@@` hunk-header line, but it does **not** insert an entry into `pm.files[currentFile][position]`. The GitHub diff-position spec — and issue #79 explicitly — defines the `@@` line as **position 1** (or the continuing counter for subsequent hunks), and it is a valid target for inline comments. Because no map entry is written for `@@` positions, `Translate()` falls into the `!ok` branch and returns a hard error (`"position N out of range for file"`), rather than falling back to the nearest non-deletion line below the header. Deletion lines already get this fallback treatment (they are stored as `-1` and Translate follows the chain). Hunk-header positions should receive the same: map them to `0` (or a sentinel) and let Translate fall through to the nearest context/addition line below. The current asymmetry means any LLM comment positioned at a `@@` line will cause `PostReview` to abort for that file's comments entirely. ## Source - PR: #90 — feat(vcs): Gitea adapter with diff-position translation (Phase 2) - Linked issue: #79 — feat(vcs): Gitea adapter with diff-position translation (Phase 2) - Code quality rule: Diff-position spec compliance - File: `gitea/position.go`, hunk-header handling block ## What needs to happen 1. In `BuildPositionToLineMap`, when a `@@` hunk-header line is encountered, store the position with a sentinel value (e.g., `0`) in `pm.files[currentFile]`: `pm.files[currentFile][position] = 0`. 2. In `Translate()`, add a branch for `lineNum == 0` that applies the same "nearest non-deletion line below" fallback used for deletion lines (`lineNum == -1`). 3. Add test cases to `position_test.go` that call `Translate(file, 1)` (single-hunk) and the continuing `@@` position in a multi-hunk diff, verifying they return the first new-file line of the hunk rather than an error. ## Acceptance criteria - `Translate(file, hunkHeaderPosition)` returns the line number of the first context/addition line in the hunk, not an error - Tests added for single-hunk and multi-hunk `@@` positions - All existing tests continue to pass ## References - [PR #90](https://gitea.weiker.me/rodin/review-bot/pulls/90) - [Issue #79](https://gitea.weiker.me/rodin/review-bot/issues/79)
rodin added the ai-reviewbug labels 2026-05-13 02:57:15 +00:00
rodin self-assigned this 2026-05-13 06:10:06 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
ai-review
bug
needs-review
ready
Ready for human review
 self-reviewed
Self-review passed since last push
 wip
Work in progress - do not start new work on this PR
No Label ai-review bug
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
security-review-bot rodin (Rodin)
No Assignees rodin
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: rodin/review-bot#97
Delete Branch "%!s()"

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?