fix(gitea): map hunk-header positions in BuildPositionToLineMap #104
Reference in New Issue
Block a user
Delete Branch "review-bot-issue-97"
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?
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.