Skip to content

fix: resolve review comment lines with blank snippets#88

Open
Code-Bai wants to merge 1 commit into
alibaba:mainfrom
Code-Bai:codex/fix-ocr-blank-line-location
Open

fix: resolve review comment lines with blank snippets#88
Code-Bai wants to merge 1 commit into
alibaba:mainfrom
Code-Bai:codex/fix-ocr-blank-line-location

Conversation

@Code-Bai

@Code-Bai Code-Bai commented Jun 9, 2026

Copy link
Copy Markdown

Description

This PR fixes line resolution for review comments when existing_code contains internal blank lines.

Previously, splitAndNormalize dropped blank lines from the LLM-provided snippet, but the resolver still required consecutive matches against diff/file lines. As a result, snippets that matched real code with blank lines could fail to resolve to the correct start_line / end_line.

This change updates the matching logic to skip blank lines on the diff/file side as well, while preserving the real start and end line numbers.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

  • go test ./internal/diff passes locally
  • Added unit coverage for resolving snippets with internal blank lines
  • Manual testing (describe below)

Checklist

  • My code follows the existing style of this project
  • I have added tests that prove my fix is effective
  • Existing and new tests pass locally

@CLAassistant

CLAassistant commented Jun 9, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 OpenCodeReview found 1 issue(s) in this PR.

  • ✅ 1 posted as inline comment(s)
  • 📝 0 posted as summary (missing line info)

Comment thread internal/diff/resolver.go
Comment on lines +160 to +162
if sideLines[j].content == "" {
continue
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential false positive matching: By skipping blank lines, this function now allows matching across arbitrary gaps of blank lines. If two unrelated code sections happen to share the same normalized content and are separated only by blank lines, they could incorrectly match. Consider adding a sanity check that the gap between start and end is not unreasonably larger than len(targetLines) — for example, ensuring (end - start + 1) <= len(targetLines) * someFactor. Without such a guard, a comment could be attached to the wrong code location in files with many blank lines.

Also, consider adding a unit test for matchConsecutive that directly tests the blank-line-skipping behavior (e.g., sideLines with interspersed empty-content entries), since the existing unit tests only cover strictly consecutive matches.

@Code-Bai Code-Bai force-pushed the codex/fix-ocr-blank-line-location branch from 992d2dc to 5d268ee Compare June 10, 2026 02:00
@Code-Bai Code-Bai force-pushed the codex/fix-ocr-blank-line-location branch from 5d268ee to 6000330 Compare June 10, 2026 02:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants