Fixes#97 — BuildPositionToLineMap incremented position and updated maxPositions for @@ hunk-header lines but did not store a map entry in pm.files[currentFile]. This caused Translate() to return a hard error ("position N out of range for file") for any LLM comment positioned at a hunk header, aborting the entire file's inline comments in PostReview.
Changes
gitea/position.go
BuildPositionToLineMap: Store sentinel value 0 for hunk-header positions (pm.files[currentFile][position] = 0), analogous to -1 for deletion lines.
Translate: Unified the fallback logic — both lineNum == -1 (deletion) and lineNum == 0 (hunk-header) now walk forward to the nearest context/addition line below. Distinct error messages preserved for each case.
Updated doc comments to document the new sentinel and behavior.
gitea/position_test.go
Added three test cases:
Single-hunk: Translate(file, 1) returns first context/addition line (16)
Multi-hunk: Second @@ position (5) returns first line of that hunk (10); first @@ also verified
New file: @@ in a new-file diff resolves to line 1
Acceptance Criteria
✅Translate(file, hunkHeaderPosition) returns the first context/addition line, not an error
✅ Tests added for single-hunk and multi-hunk @@ positions
✅ All existing tests continue to pass
## Summary
Fixes #97 — `BuildPositionToLineMap` incremented `position` and updated `maxPositions` for `@@` hunk-header lines but did not store a map entry in `pm.files[currentFile]`. This caused `Translate()` to return a hard error (`"position N out of range for file"`) for any LLM comment positioned at a hunk header, aborting the entire file's inline comments in `PostReview`.
## Changes
### `gitea/position.go`
1. **`BuildPositionToLineMap`**: Store sentinel value `0` for hunk-header positions (`pm.files[currentFile][position] = 0`), analogous to `-1` for deletion lines.
2. **`Translate`**: Unified the fallback logic — both `lineNum == -1` (deletion) and `lineNum == 0` (hunk-header) now walk forward to the nearest context/addition line below. Distinct error messages preserved for each case.
3. Updated doc comments to document the new sentinel and behavior.
### `gitea/position_test.go`
Added three test cases:
- **Single-hunk**: `Translate(file, 1)` returns first context/addition line (16)
- **Multi-hunk**: Second `@@` position (5) returns first line of that hunk (10); first `@@` also verified
- **New file**: `@@` in a new-file diff resolves to line 1
## Acceptance Criteria
- ✅ `Translate(file, hunkHeaderPosition)` returns the first context/addition line, not an error
- ✅ Tests added for single-hunk and multi-hunk `@@` positions
- ✅ All existing tests continue to pass
rodin
added the wip label 2026-05-13 06:13:45 +00:00
BuildPositionToLineMap incremented position and updated maxPositions for
@@ hunk-header lines but did not store a map entry, causing Translate()
to return a hard error for any comment positioned at a hunk header.
Store sentinel value 0 for hunk-header positions (analogous to -1 for
deletions) and extend Translate() to fall through to the nearest
context/addition line below, matching the existing deletion-line
behavior.
Fixes#97
rodin
removed the wip label 2026-05-13 06:13:50 +00:00
Clean, focused fix for the hunk-header position bug. The sentinel value approach (0 for hunk-headers, -1 for deletions) is consistent with the existing pattern, the unified fallback logic in Translate is correct, and the three new test cases cover the meaningful scenarios. CI passes.
Findings
#
Severity
File
Line
Finding
1
[NIT]
gitea/position.go
55
The error-message discrimination between lineNum==0 and lineNum==-1 is checked after the walk loop, meaning the if lineNum == 0 branch is dead code for the hunk-header error path when there IS a subsequent line — that path returns early. The logic is still correct, but the structure is slightly misleading: the if/else at lines 53-57 would be clearer if written as two separate if lineNum == -1 / if lineNum == 0 fallbacks before the shared loop, or the error strings could simply be unified into one message since both cases have identical semantics from a caller perspective. Minor readability issue only.
2
[NIT]
gitea/position_test.go
308
TestTranslate_HunkHeaderPosition_MultiHunk does not test the edge case where a hunk-header is the last position in the file (i.e., the error path for a hunk-header with no subsequent line). The deletion equivalent is covered by TestBuildPositionToLineMap_DeletionAtEnd but there is no corresponding test for the new hunk-header error path. Not a correctness issue since the code path is trivially reached from the shared loop, but worth noting for completeness.
Recommendation
APPROVE — Approve. The fix is correct and minimal: it adds the missing map entry for hunk-header lines (the root cause of the bug), unifies the fallback walk in Translate, and preserves distinct error messages. Both nits are cosmetic — the logic is sound. No blocking issues.
~~Original review~~
**Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/104#pullrequestreview-3043) for up-to-date findings.
<details><summary>Previous findings (commit 23dc7819)</summary>
# Sonnet Review
## Summary
Clean, focused fix for the hunk-header position bug. The sentinel value approach (0 for hunk-headers, -1 for deletions) is consistent with the existing pattern, the unified fallback logic in Translate is correct, and the three new test cases cover the meaningful scenarios. CI passes.
## Findings
| # | Severity | File | Line | Finding |
|---|----------|------|------|--------|
| 1 | [NIT] | `gitea/position.go` | 55 | The error-message discrimination between lineNum==0 and lineNum==-1 is checked after the walk loop, meaning the `if lineNum == 0` branch is dead code for the hunk-header error path when there IS a subsequent line — that path returns early. The logic is still correct, but the structure is slightly misleading: the if/else at lines 53-57 would be clearer if written as two separate `if lineNum == -1` / `if lineNum == 0` fallbacks before the shared loop, or the error strings could simply be unified into one message since both cases have identical semantics from a caller perspective. Minor readability issue only. |
| 2 | [NIT] | `gitea/position_test.go` | 308 | TestTranslate_HunkHeaderPosition_MultiHunk does not test the edge case where a hunk-header is the last position in the file (i.e., the error path for a hunk-header with no subsequent line). The deletion equivalent is covered by TestBuildPositionToLineMap_DeletionAtEnd but there is no corresponding test for the new hunk-header error path. Not a correctness issue since the code path is trivially reached from the shared loop, but worth noting for completeness. |
## Recommendation
**APPROVE** — Approve. The fix is correct and minimal: it adds the missing map entry for hunk-header lines (the root cause of the bug), unifies the fallback walk in Translate, and preserves distinct error messages. Both nits are cosmetic — the logic is sound. No blocking issues.
---
*Review by sonnet*
<!-- review-bot:sonnet -->
---
*Evaluated against 23dc7819*
</details>
<!-- review-bot:sonnet -->
[NIT] The error-message discrimination between lineNum==0 and lineNum==-1 is checked after the walk loop, meaning the if lineNum == 0 branch is dead code for the hunk-header error path when there IS a subsequent line — that path returns early. The logic is still correct, but the structure is slightly misleading: the if/else at lines 53-57 would be clearer if written as two separate if lineNum == -1 / if lineNum == 0 fallbacks before the shared loop, or the error strings could simply be unified into one message since both cases have identical semantics from a caller perspective. Minor readability issue only.
**[NIT]** The error-message discrimination between lineNum==0 and lineNum==-1 is checked after the walk loop, meaning the `if lineNum == 0` branch is dead code for the hunk-header error path when there IS a subsequent line — that path returns early. The logic is still correct, but the structure is slightly misleading: the if/else at lines 53-57 would be clearer if written as two separate `if lineNum == -1` / `if lineNum == 0` fallbacks before the shared loop, or the error strings could simply be unified into one message since both cases have identical semantics from a caller perspective. Minor readability issue only.
[NIT] TestTranslate_HunkHeaderPosition_MultiHunk does not test the edge case where a hunk-header is the last position in the file (i.e., the error path for a hunk-header with no subsequent line). The deletion equivalent is covered by TestBuildPositionToLineMap_DeletionAtEnd but there is no corresponding test for the new hunk-header error path. Not a correctness issue since the code path is trivially reached from the shared loop, but worth noting for completeness.
**[NIT]** TestTranslate_HunkHeaderPosition_MultiHunk does not test the edge case where a hunk-header is the last position in the file (i.e., the error path for a hunk-header with no subsequent line). The deletion equivalent is covered by TestBuildPositionToLineMap_DeletionAtEnd but there is no corresponding test for the new hunk-header error path. Not a correctness issue since the code path is trivially reached from the shared loop, but worth noting for completeness.
security-review-bot
requested review from security-review-bot 2026-05-13 06:14:46 +00:00
The changes correctly handle hunk-header positions by mapping them to a sentinel and updating Translate to resolve to the next applicable line, with comprehensive tests added. No security concerns are introduced by these modifications.
Recommendation
APPROVE — The implementation aligns with safer handling of diff positions and avoids aborting on hunk headers, with clear error messages and additional test coverage. CI has passed and there are no security-relevant issues in the changes. Approve as-is.
~~Original review~~
**Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/104#pullrequestreview-3045) for up-to-date findings.
<details><summary>Previous findings (commit 23dc7819)</summary>
# Security Review
## Summary
The changes correctly handle hunk-header positions by mapping them to a sentinel and updating Translate to resolve to the next applicable line, with comprehensive tests added. No security concerns are introduced by these modifications.
## Recommendation
**APPROVE** — The implementation aligns with safer handling of diff positions and avoids aborting on hunk headers, with clear error messages and additional test coverage. CI has passed and there are no security-relevant issues in the changes. Approve as-is.
---
*Review by security*
<!-- review-bot:security -->
---
*Evaluated against 23dc7819*
</details>
<!-- review-bot:security -->
gpt-review-bot
approved these changes 2026-05-13 06:16:04 +00:00
Change correctly handles @@ hunk-header positions by mapping them to a sentinel and updating Translate to find the next valid line. Tests comprehensively cover single- and multi-hunk cases and new files, and CI has passed.
Recommendation
APPROVE — The implementation is sound and aligns with the documented behavior: hunk headers now map to 0 and Translate properly advances to the next context/addition line. Error messages remain clear, and added tests cover the intended scenarios without breaking existing behavior. Given CI success and no identified major issues, this is ready to merge.
~~Original review~~
**Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/104#pullrequestreview-3046) for up-to-date findings.
<details><summary>Previous findings (commit 23dc7819)</summary>
# Gpt Review
## Summary
Change correctly handles @@ hunk-header positions by mapping them to a sentinel and updating Translate to find the next valid line. Tests comprehensively cover single- and multi-hunk cases and new files, and CI has passed.
## Recommendation
**APPROVE** — The implementation is sound and aligns with the documented behavior: hunk headers now map to 0 and Translate properly advances to the next context/addition line. Error messages remain clear, and added tests cover the intended scenarios without breaking existing behavior. Given CI success and no identified major issues, this is ready to merge.
---
*Review by gpt*
<!-- review-bot:gpt -->
---
*Evaluated against 23dc7819*
</details>
<!-- review-bot:gpt -->
rodin
added the wip label 2026-05-13 06:20:17 +00:00
None — diff looks clean. The sentinel value approach (0 for hunk-headers, -1 for deletions) is consistent and the unified lineNum <= 0 fallback correctly handles both cases. Error messages remain distinct for debugging purposes. Tests cover single-hunk, multi-hunk, and new-file scenarios.
One observation (not a finding): there's no test for the hunk-header error path (hunk-header as the last position with no subsequent line). This is cosmetic — the code path is trivially correct from inspection and mirrors the existing deletion-at-end test pattern.
Phase 2: Prior Review Verification
Finding
Reviewer
Status
Notes
Error-message structure after walk loop slightly misleading (NIT)
sonnet-review-bot
🚫 Invalid
The structure is intentional — it provides distinct, actionable error messages for callers while sharing the walk loop. Refactoring into separate pre-loop checks would duplicate the loop or add unnecessary complexity. Current form is idiomatic Go.
No test for hunk-header error path when it's the last position (NIT)
sonnet-review-bot
⚠️ Partial
Not addressed but the impact is negligible — the code path is trivially reachable and mirrors the existing deletion-at-end pattern. Could be added for completeness but not blocking.
Security review: no concerns
security-review-bot
✅ Resolved
N/A
GPT review: no issues found
gpt-review-bot
✅ Resolved
N/A
Assessment: ✅ Clean
All three bot reviewers approved. The only unaddressed item is a NIT requesting an additional test for the hunk-header error path (no subsequent line). This is cosmetic — the code path is trivially correct and the existing test pattern for deletions demonstrates equivalent coverage. No correctness, performance, or security concerns. Ready for merge.
## Self-Review: PR #104
Self-review against 23dc781908541cd9971785818dc5bde4f5fb71f7
### Phase 1: Independent Findings
None — diff looks clean. The sentinel value approach (0 for hunk-headers, -1 for deletions) is consistent and the unified `lineNum <= 0` fallback correctly handles both cases. Error messages remain distinct for debugging purposes. Tests cover single-hunk, multi-hunk, and new-file scenarios.
One observation (not a finding): there's no test for the hunk-header error path (hunk-header as the last position with no subsequent line). This is cosmetic — the code path is trivially correct from inspection and mirrors the existing deletion-at-end test pattern.
### Phase 2: Prior Review Verification
| Finding | Reviewer | Status | Notes |
|---------|----------|--------|-------|
| Error-message structure after walk loop slightly misleading (NIT) | sonnet-review-bot | 🚫 Invalid | The structure is intentional — it provides distinct, actionable error messages for callers while sharing the walk loop. Refactoring into separate pre-loop checks would duplicate the loop or add unnecessary complexity. Current form is idiomatic Go. |
| No test for hunk-header error path when it's the last position (NIT) | sonnet-review-bot | ⚠️ Partial | Not addressed but the impact is negligible — the code path is trivially reachable and mirrors the existing deletion-at-end pattern. Could be added for completeness but not blocking. |
| Security review: no concerns | security-review-bot | ✅ Resolved | N/A |
| GPT review: no issues found | gpt-review-bot | ✅ Resolved | N/A |
### Assessment: ✅ Clean
All three bot reviewers approved. The only unaddressed item is a NIT requesting an additional test for the hunk-header error path (no subsequent line). This is cosmetic — the code path is trivially correct and the existing test pattern for deletions demonstrates equivalent coverage. No correctness, performance, or security concerns. Ready for merge.
rodin
removed the wip label 2026-05-13 06:21:54 +00:00
aweiker
was assigned by rodin2026-05-13 06:25:31 +00:00
rodin
added the ready label 2026-05-13 06:25:31 +00:00
rodin
removed the ready label 2026-05-13 06:30:41 +00:00
aweiker
was unassigned by rodin2026-05-13 06:30:41 +00:00
rodin
self-assigned this 2026-05-13 06:30:41 +00:00
rodin
added the wip label 2026-05-13 06:30:46 +00:00
Adds TestTranslate_HunkHeaderAtEnd covering the edge case where a
hunk-header is the last position in the file with no subsequent
new-file line. Mirrors TestBuildPositionToLineMap_DeletionAtEnd for
the hunk-header code path.
Addresses NIT from sonnet-review-bot on PR #104 (comment 18412).
The if/else structure is intentional idiomatic Go: a single if lineNum <= 0 guard gates the shared forward-walk logic, and the error messages at the bottom differentiate between hunk-header (lineNum == 0) and deletion (lineNum == -1) for debugging clarity. Unifying the messages would lose that diagnostic signal. Splitting into separate pre-loop blocks would duplicate the walk logic.
The reviewer's observation that the lineNum == 0 branch is "dead code for the hunk-header path when there IS a subsequent line" is the intended behavior — that's the success path returning early from the loop. The error discrimination only fires on failure, which is correct.
No change made.
Re: [NIT] comment 18411 on gitea/position.go — error-message structure
Acknowledged — I'll push back on this one.
The if/else structure is intentional idiomatic Go: a single `if lineNum <= 0` guard gates the shared forward-walk logic, and the error messages at the bottom differentiate between hunk-header (lineNum == 0) and deletion (lineNum == -1) for debugging clarity. Unifying the messages would lose that diagnostic signal. Splitting into separate pre-loop blocks would duplicate the walk logic.
The reviewer's observation that the `lineNum == 0` branch is "dead code for the hunk-header path when there IS a subsequent line" is the intended behavior — that's the success path returning early from the loop. The error discrimination only fires on failure, which is correct.
No change made.
---
*Re: [NIT] comment 18411 on `gitea/position.go` — error-message structure*
Good catch — added TestTranslate_HunkHeaderAtEnd in commit 02bdd70. This covers the edge case where a hunk-header is the last position in the file with no subsequent new-file line, mirroring TestBuildPositionToLineMap_DeletionAtEnd for the hunk-header code path.
Re: [NIT] comment 18412 on gitea/position_test.go — missing hunk-header error path test
Good catch — added `TestTranslate_HunkHeaderAtEnd` in commit 02bdd70. This covers the edge case where a hunk-header is the last position in the file with no subsequent new-file line, mirroring `TestBuildPositionToLineMap_DeletionAtEnd` for the hunk-header code path.
---
*Re: [NIT] comment 18412 on `gitea/position_test.go` — missing hunk-header error path test*
The fix is correct, minimal, and well-tested. It stores a sentinel value of 0 for hunk-header positions in BuildPositionToLineMap, then unifies the fallback logic in Translate to handle both deletion (-1) and hunk-header (0) sentinels by walking forward to the nearest context/addition line. The four new test cases cover the main scenarios including the error path.
Findings
#
Severity
File
Line
Finding
1
[NIT]
gitea/position.go
52
The error-message disambiguation (hunk-header vs deletion) occurs after the forward-scan loop, but the check if lineNum == 0 is evaluated only after the loop exhausts. This is correct and readable, but the two error messages could equally be computed before the loop (since lineNum doesn't change). Minor style point only — no functional issue.
Recommendation
APPROVE — The change is correct and complete. CI passes, the sentinel-value approach is consistent with the existing deletion-line convention, the new Translate fallback correctly unifies both zero-or-negative sentinels, and the four new tests cover single-hunk, multi-hunk, new-file, and the error path. The only nit is cosmetic. Approve as-is.
# Sonnet Review
## Summary
The fix is correct, minimal, and well-tested. It stores a sentinel value of 0 for hunk-header positions in BuildPositionToLineMap, then unifies the fallback logic in Translate to handle both deletion (-1) and hunk-header (0) sentinels by walking forward to the nearest context/addition line. The four new test cases cover the main scenarios including the error path.
## Findings
| # | Severity | File | Line | Finding |
|---|----------|------|------|--------|
| 1 | [NIT] | `gitea/position.go` | 52 | The error-message disambiguation (hunk-header vs deletion) occurs after the forward-scan loop, but the check `if lineNum == 0` is evaluated only after the loop exhausts. This is correct and readable, but the two error messages could equally be computed before the loop (since `lineNum` doesn't change). Minor style point only — no functional issue. |
## Recommendation
**APPROVE** — The change is correct and complete. CI passes, the sentinel-value approach is consistent with the existing deletion-line convention, the new `Translate` fallback correctly unifies both zero-or-negative sentinels, and the four new tests cover single-hunk, multi-hunk, new-file, and the error path. The only nit is cosmetic. Approve as-is.
---
*Review by sonnet*
<!-- review-bot:sonnet -->
---
*Evaluated against 02bdd701*
[NIT] The error-message disambiguation (hunk-header vs deletion) occurs after the forward-scan loop, but the check if lineNum == 0 is evaluated only after the loop exhausts. This is correct and readable, but the two error messages could equally be computed before the loop (since lineNum doesn't change). Minor style point only — no functional issue.
**[NIT]** The error-message disambiguation (hunk-header vs deletion) occurs after the forward-scan loop, but the check `if lineNum == 0` is evaluated only after the loop exhausts. This is correct and readable, but the two error messages could equally be computed before the loop (since `lineNum` doesn't change). Minor style point only — no functional issue.
security-review-bot
requested review from security-review-bot 2026-05-13 06:33:22 +00:00
Changes correctly add a sentinel for hunk-header positions and unify fallback resolution in Translate without introducing security risks. CI passed and tests cover the new behavior comprehensively.
Recommendation
APPROVE — The update is safe and improves robustness when handling hunk headers by preventing hard errors on those positions. No security vulnerabilities are introduced by the changes. Proceed with merge.
# Security Review
## Summary
Changes correctly add a sentinel for hunk-header positions and unify fallback resolution in Translate without introducing security risks. CI passed and tests cover the new behavior comprehensively.
## Recommendation
**APPROVE** — The update is safe and improves robustness when handling hunk headers by preventing hard errors on those positions. No security vulnerabilities are introduced by the changes. Proceed with merge.
---
*Review by security*
<!-- review-bot:security -->
---
*Evaluated against 02bdd701*
None — diff looks clean. The new test follows the same pattern as TestBuildPositionToLineMap_DeletionAtEnd and correctly exercises the hunk-header error path.
Both NIT findings addressed — one via push-back with justification, one with a new test. All tests pass. No new issues found in the diff.
## Self-Review: PR #104
Self-review against 02bdd701a56651aeb1ae7a9e35e2f20b3ed055f9
### Phase 1: Independent Findings
None — diff looks clean. The new test follows the same pattern as `TestBuildPositionToLineMap_DeletionAtEnd` and correctly exercises the hunk-header error path.
### Phase 2: Prior Review Verification
| Finding | Reviewer | Status | Notes |
|---------|----------|--------|-------|
| NIT: error-message structure misleading (comment 18411) | sonnet-review-bot | ✅ Pushed back | Idiomatic Go — single guard, differentiated errors for debug clarity |
| NIT: missing hunk-header-at-end test (comment 18412) | sonnet-review-bot | ✅ Fixed | Added `TestTranslate_HunkHeaderAtEnd` in 02bdd70 |
### Assessment: ✅ Clean
Both NIT findings addressed — one via push-back with justification, one with a new test. All tests pass. No new issues found in the diff.
rodin
removed the wip label 2026-05-13 06:33:31 +00:00
gpt-review-bot
approved these changes 2026-05-13 06:34:13 +00:00
The change correctly adds sentinel mapping for hunk-header positions and unifies Translate's fallback to resolve to the next context/addition line. Tests comprehensively cover single-hunk, multi-hunk, new-file, and edge cases; CI passed.
Recommendation
APPROVE — The implementation is sound and idiomatic: hunk headers are now represented with a sentinel (0), Translate gracefully handles both deletions and headers, and error messages remain descriptive. The added tests validate expected behavior across scenarios, and existing behavior remains intact. Proceed with merge.
# Gpt Review
## Summary
The change correctly adds sentinel mapping for hunk-header positions and unifies Translate's fallback to resolve to the next context/addition line. Tests comprehensively cover single-hunk, multi-hunk, new-file, and edge cases; CI passed.
## Recommendation
**APPROVE** — The implementation is sound and idiomatic: hunk headers are now represented with a sentinel (0), Translate gracefully handles both deletions and headers, and error messages remain descriptive. The added tests validate expected behavior across scenarios, and existing behavior remains intact. Proceed with merge.
---
*Review by gpt*
<!-- review-bot:gpt -->
---
*Evaluated against 02bdd701*
rodin
removed their assignment 2026-05-13 06:35:45 +00:00
aweiker
was assigned by rodin2026-05-13 06:35:45 +00:00
rodin
added the ready label 2026-05-13 06:35:45 +00:00
rodin
removed the ready label 2026-05-13 11:06:37 +00:00
aweiker
was unassigned by rodin2026-05-13 11:06:37 +00:00
rodin
self-assigned this 2026-05-13 11:06:37 +00:00
rodin
added the wip label 2026-05-13 11:06:42 +00:00
Same reasoning as the earlier NIT on this topic: the post-loop position is the natural place for error differentiation. Computing it before the loop would add a variable that's only used in the failure path and separate the error logic from where the failure is detected. Current flow is: attempt resolution → explain failure. Keeping as-is.
Same reasoning as the earlier NIT on this topic: the post-loop position is the natural place for error differentiation. Computing it before the loop would add a variable that's only used in the failure path and separate the error logic from where the failure is detected. Current flow is: attempt resolution → explain failure. Keeping as-is.
Acknowledged. The current structure reads top-to-bottom: scan forward for a resolution, then differentiate the error. Moving the check before the loop would require storing the message in a variable or duplicating the scan logic. The linear post-loop flow is clearer. Keeping as-is.
Acknowledged. The current structure reads top-to-bottom: scan forward for a resolution, then differentiate the error. Moving the check before the loop would require storing the message in a variable or duplicating the scan logic. The linear post-loop flow is clearer. Keeping as-is.
This edge case IS tested — see TestTranslate_HunkHeaderAtEnd (line 361), which tests exactly the scenario where a hunk-header is the last position with no subsequent new-file line. That test covers the error path the NIT mentions.
This edge case IS tested — see `TestTranslate_HunkHeaderAtEnd` (line 361), which tests exactly the scenario where a hunk-header is the last position with no subsequent new-file line. That test covers the error path the NIT mentions.
rodin
removed the wip label 2026-05-13 11:09:08 +00:00
rodin
added the ready label 2026-05-13 11:13:42 +00:00
rodin
removed their assignment 2026-05-13 11:16:40 +00:00
aweiker
was assigned by rodin2026-05-13 11:16:40 +00:00
aweiker
approved these changes 2026-05-13 13:15:28 +00:00
aweiker
merged commit 65ba8af244 into feature/github-support2026-05-13 13:15:31 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
sonnet-review-bot
security-review-bot
gpt-review-bot
rodin
aweiker
Summary
Fixes #97 —
BuildPositionToLineMapincrementedpositionand updatedmaxPositionsfor@@hunk-header lines but did not store a map entry inpm.files[currentFile]. This causedTranslate()to return a hard error ("position N out of range for file") for any LLM comment positioned at a hunk header, aborting the entire file's inline comments inPostReview.Changes
gitea/position.goBuildPositionToLineMap: Store sentinel value0for hunk-header positions (pm.files[currentFile][position] = 0), analogous to-1for deletion lines.Translate: Unified the fallback logic — bothlineNum == -1(deletion) andlineNum == 0(hunk-header) now walk forward to the nearest context/addition line below. Distinct error messages preserved for each case.gitea/position_test.goAdded three test cases:
Translate(file, 1)returns first context/addition line (16)@@position (5) returns first line of that hunk (10); first@@also verified@@in a new-file diff resolves to line 1Acceptance Criteria
Translate(file, hunkHeaderPosition)returns the first context/addition line, not an error@@positionsOriginal reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
23dc7819)Sonnet Review
Summary
Clean, focused fix for the hunk-header position bug. The sentinel value approach (0 for hunk-headers, -1 for deletions) is consistent with the existing pattern, the unified fallback logic in Translate is correct, and the three new test cases cover the meaningful scenarios. CI passes.
Findings
gitea/position.goif lineNum == 0branch is dead code for the hunk-header error path when there IS a subsequent line — that path returns early. The logic is still correct, but the structure is slightly misleading: the if/else at lines 53-57 would be clearer if written as two separateif lineNum == -1/if lineNum == 0fallbacks before the shared loop, or the error strings could simply be unified into one message since both cases have identical semantics from a caller perspective. Minor readability issue only.gitea/position_test.goRecommendation
APPROVE — Approve. The fix is correct and minimal: it adds the missing map entry for hunk-header lines (the root cause of the bug), unifies the fallback walk in Translate, and preserves distinct error messages. Both nits are cosmetic — the logic is sound. No blocking issues.
Review by sonnet
Evaluated against
23dc7819@@ -50,2 +52,4 @@}}if lineNum == 0 {return 0, fmt.Errorf("position %d targets a hunk-header line with no subsequent new-file line in %q", position, file)[NIT] The error-message discrimination between lineNum==0 and lineNum==-1 is checked after the walk loop, meaning the
if lineNum == 0branch is dead code for the hunk-header error path when there IS a subsequent line — that path returns early. The logic is still correct, but the structure is slightly misleading: the if/else at lines 53-57 would be clearer if written as two separateif lineNum == -1/if lineNum == 0fallbacks before the shared loop, or the error strings could simply be unified into one message since both cases have identical semantics from a caller perspective. Minor readability issue only.@@ -275,0 +305,4 @@--- a/file.go+++ b/file.go@@ -1,3 +1,3 @@ package mainline1[NIT] TestTranslate_HunkHeaderPosition_MultiHunk does not test the edge case where a hunk-header is the last position in the file (i.e., the error path for a hunk-header with no subsequent line). The deletion equivalent is covered by TestBuildPositionToLineMap_DeletionAtEnd but there is no corresponding test for the new hunk-header error path. Not a correctness issue since the code path is trivially reached from the shared loop, but worth noting for completeness.
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
23dc7819)Security Review
Summary
The changes correctly handle hunk-header positions by mapping them to a sentinel and updating Translate to resolve to the next applicable line, with comprehensive tests added. No security concerns are introduced by these modifications.
Recommendation
APPROVE — The implementation aligns with safer handling of diff positions and avoids aborting on hunk headers, with clear error messages and additional test coverage. CI has passed and there are no security-relevant issues in the changes. Approve as-is.
Review by security
Evaluated against
23dc7819Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
23dc7819)Gpt Review
Summary
Change correctly handles @@ hunk-header positions by mapping them to a sentinel and updating Translate to find the next valid line. Tests comprehensively cover single- and multi-hunk cases and new files, and CI has passed.
Recommendation
APPROVE — The implementation is sound and aligns with the documented behavior: hunk headers now map to 0 and Translate properly advances to the next context/addition line. Error messages remain clear, and added tests cover the intended scenarios without breaking existing behavior. Given CI success and no identified major issues, this is ready to merge.
Review by gpt
Evaluated against
23dc7819Self-Review: PR #104
Self-review against
23dc781908Phase 1: Independent Findings
None — diff looks clean. The sentinel value approach (0 for hunk-headers, -1 for deletions) is consistent and the unified
lineNum <= 0fallback correctly handles both cases. Error messages remain distinct for debugging purposes. Tests cover single-hunk, multi-hunk, and new-file scenarios.One observation (not a finding): there's no test for the hunk-header error path (hunk-header as the last position with no subsequent line). This is cosmetic — the code path is trivially correct from inspection and mirrors the existing deletion-at-end test pattern.
Phase 2: Prior Review Verification
Assessment: ✅ Clean
All three bot reviewers approved. The only unaddressed item is a NIT requesting an additional test for the hunk-header error path (no subsequent line). This is cosmetic — the code path is trivially correct and the existing test pattern for deletions demonstrates equivalent coverage. No correctness, performance, or security concerns. Ready for merge.
Acknowledged — I'll push back on this one.
The if/else structure is intentional idiomatic Go: a single
if lineNum <= 0guard gates the shared forward-walk logic, and the error messages at the bottom differentiate between hunk-header (lineNum == 0) and deletion (lineNum == -1) for debugging clarity. Unifying the messages would lose that diagnostic signal. Splitting into separate pre-loop blocks would duplicate the walk logic.The reviewer's observation that the
lineNum == 0branch is "dead code for the hunk-header path when there IS a subsequent line" is the intended behavior — that's the success path returning early from the loop. The error discrimination only fires on failure, which is correct.No change made.
Re: [NIT] comment 18411 on
gitea/position.go— error-message structureGood catch — added
TestTranslate_HunkHeaderAtEndin commit02bdd70. This covers the edge case where a hunk-header is the last position in the file with no subsequent new-file line, mirroringTestBuildPositionToLineMap_DeletionAtEndfor the hunk-header code path.Re: [NIT] comment 18412 on
gitea/position_test.go— missing hunk-header error path testSonnet Review
Summary
The fix is correct, minimal, and well-tested. It stores a sentinel value of 0 for hunk-header positions in BuildPositionToLineMap, then unifies the fallback logic in Translate to handle both deletion (-1) and hunk-header (0) sentinels by walking forward to the nearest context/addition line. The four new test cases cover the main scenarios including the error path.
Findings
gitea/position.goif lineNum == 0is evaluated only after the loop exhausts. This is correct and readable, but the two error messages could equally be computed before the loop (sincelineNumdoesn't change). Minor style point only — no functional issue.Recommendation
APPROVE — The change is correct and complete. CI passes, the sentinel-value approach is consistent with the existing deletion-line convention, the new
Translatefallback correctly unifies both zero-or-negative sentinels, and the four new tests cover single-hunk, multi-hunk, new-file, and the error path. The only nit is cosmetic. Approve as-is.Review by sonnet
Evaluated against
02bdd701@@ -47,4 +49,4 @@for p := position + 1; p <= maxPos; p++ {if ln, exists := fileMap[p]; exists && ln > 0 {return ln, nil}[NIT] The error-message disambiguation (hunk-header vs deletion) occurs after the forward-scan loop, but the check
if lineNum == 0is evaluated only after the loop exhausts. This is correct and readable, but the two error messages could equally be computed before the loop (sincelineNumdoesn't change). Minor style point only — no functional issue.Security Review
Summary
Changes correctly add a sentinel for hunk-header positions and unify fallback resolution in Translate without introducing security risks. CI passed and tests cover the new behavior comprehensively.
Recommendation
APPROVE — The update is safe and improves robustness when handling hunk headers by preventing hard errors on those positions. No security vulnerabilities are introduced by the changes. Proceed with merge.
Review by security
Evaluated against
02bdd701Self-Review: PR #104
Self-review against
02bdd701a5Phase 1: Independent Findings
None — diff looks clean. The new test follows the same pattern as
TestBuildPositionToLineMap_DeletionAtEndand correctly exercises the hunk-header error path.Phase 2: Prior Review Verification
TestTranslate_HunkHeaderAtEndin02bdd70Assessment: ✅ Clean
Both NIT findings addressed — one via push-back with justification, one with a new test. All tests pass. No new issues found in the diff.
Gpt Review
Summary
The change correctly adds sentinel mapping for hunk-header positions and unifies Translate's fallback to resolve to the next context/addition line. Tests comprehensively cover single-hunk, multi-hunk, new-file, and edge cases; CI passed.
Recommendation
APPROVE — The implementation is sound and idiomatic: hunk headers are now represented with a sentinel (0), Translate gracefully handles both deletions and headers, and error messages remain descriptive. The added tests validate expected behavior across scenarios, and existing behavior remains intact. Proceed with merge.
Review by gpt
Evaluated against
02bdd701Addressing review NITs — pushing back on all three as the code structure is intentional and the missing test already exists.
@@ -46,2 +46,4 @@// Both map to the nearest context/addition line below.if lineNum <= 0 {maxPos := pm.maxPosition(file)for p := position + 1; p <= maxPos; p++ {Same reasoning as the earlier NIT on this topic: the post-loop position is the natural place for error differentiation. Computing it before the loop would add a variable that's only used in the failure path and separate the error logic from where the failure is detected. Current flow is: attempt resolution → explain failure. Keeping as-is.
@@ -48,4 +50,4 @@if ln, exists := fileMap[p]; exists && ln > 0 {return ln, nil}}Acknowledged. The current structure reads top-to-bottom: scan forward for a resolution, then differentiate the error. Moving the check before the loop would require storing the message in a variable or duplicating the scan logic. The linear post-loop flow is clearer. Keeping as-is.
@@ -275,0 +302,4 @@// Second hunk: @@ is pos 5, then " func foo() {" (pos 6), "+// added" (pos 7), etc.// Translating position 5 (second @@) should resolve to new line 10.diff := `diff --git a/file.go b/file.go--- a/file.goThis edge case IS tested — see
TestTranslate_HunkHeaderAtEnd(line 361), which tests exactly the scenario where a hunk-header is the last position with no subsequent new-file line. That test covers the error path the NIT mentions.