fix(gitea): map hunk-header positions in BuildPositionToLineMap #104

Merged
aweiker merged 2 commits from review-bot-issue-97 into feature/github-support 2026-05-13 13:15:31 +00:00
Owner

Summary

Fixes #97BuildPositionToLineMap 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
## 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
rodin added 1 commit 2026-05-13 06:13:46 +00:00
fix(gitea): map hunk-header positions in BuildPositionToLineMap
CI / test (pull_request) Successful in 28s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 27s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 44s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m49s
23dc781908
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
sonnet-review-bot approved these changes 2026-05-13 06:14:41 +00:00
sonnet-review-bot left a comment
First-time contributor

Original review

Supersededsee 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

# 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


Evaluated against 23dc7819

~~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 -->
@@ -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)
First-time contributor

[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.
@@ -275,0 +305,4 @@
--- a/file.go
+++ b/file.go
@@ -1,3 +1,3 @@ package main
line1
First-time contributor

[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
security-review-bot approved these changes 2026-05-13 06:14:46 +00:00
Dismissed
security-review-bot left a comment
Collaborator

Original review

Supersededsee 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 23dc7819

~~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
Dismissed
gpt-review-bot left a comment
First-time contributor

Original review

Supersededsee 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 23dc7819

~~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
Author
Owner

Self-Review: PR #104

Self-review against 23dc781908

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.

## 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 rodin 2026-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 rodin 2026-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
rodin added 1 commit 2026-05-13 06:32:27 +00:00
test(gitea): add hunk-header-at-end error path test
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 20s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 23s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 34s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m39s
02bdd701a5
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).
Author
Owner

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

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*
Author
Owner

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*
sonnet-review-bot approved these changes 2026-05-13 06:33:09 +00:00
sonnet-review-bot left a comment
First-time contributor

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


Evaluated against 02bdd701

# 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*
@@ -47,4 +49,4 @@
for p := position + 1; p <= maxPos; p++ {
if ln, exists := fileMap[p]; exists && ln > 0 {
return ln, nil
}
First-time contributor

[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
security-review-bot approved these changes 2026-05-13 06:33:22 +00:00
security-review-bot left a comment
Collaborator

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 02bdd701

# 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*
Author
Owner

Self-Review: PR #104

Self-review against 02bdd701a5

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.

## 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
gpt-review-bot left a comment
First-time contributor

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 02bdd701

# 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 rodin 2026-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 rodin 2026-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
rodin reviewed 2026-05-13 11:08:55 +00:00
rodin left a comment
Author
Owner

Addressing review NITs — pushing back on all three as the code structure is intentional and the missing test already exists.

Addressing 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++ {
Author
Owner

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.
@@ -48,4 +50,4 @@
if ln, exists := fileMap[p]; exists && ln > 0 {
return ln, nil
}
}
Author
Owner

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.
@@ -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.go
Author
Owner

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 rodin 2026-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-support 2026-05-13 13:15:31 +00:00
Sign in to join this conversation.
5 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: rodin/review-bot#104