fix(diff): disambiguate duplicate comment anchors - #992
Conversation
|
🔍 OpenCodeReview found 3 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| func ParseCandidateID(content string) (string, bool) { | ||
| content = strings.TrimSpace(content) | ||
| if content == "" { | ||
| return "", false | ||
| } | ||
| var payload map[string]json.RawMessage | ||
| if err := json.Unmarshal([]byte(content), &payload); err == nil && payload != nil { |
There was a problem hiding this comment.
Potential issue: ParseCandidateID does not handle markdown code-fence wrappers.
LLMs frequently wrap JSON output in code fences (e.g., ```json\n{"candidate_id": 1}\n```) despite explicit instructions not to. The existing ReLocateComment function calls extractCodeBlock(resp.Content()) before parsing the LLM output, but ReLocateCommentCandidate passes resp.Content() directly to ParseCandidateID without stripping fences. This means a perfectly valid candidate response wrapped in fences will fail to parse, trigger an unnecessary retry, and waste an LLM round-trip (or fail outright after exhausting retries).
Consider stripping a code-fence wrapper before JSON unmarshal, for example by trying extractCodeBlock first and falling back to the raw content, similar to how ReLocateComment handles its response.
Suggestion:
| func ParseCandidateID(content string) (string, bool) { | |
| content = strings.TrimSpace(content) | |
| if content == "" { | |
| return "", false | |
| } | |
| var payload map[string]json.RawMessage | |
| if err := json.Unmarshal([]byte(content), &payload); err == nil && payload != nil { | |
| func ParseCandidateID(content string) (string, bool) { | |
| content = strings.TrimSpace(content) | |
| if content == "" { | |
| return "", false | |
| } | |
| // LLMs sometimes wrap JSON in code fences despite being told not to. | |
| if block := extractCodeBlock(content); block != "" { | |
| content = block | |
| } | |
| var payload map[string]json.RawMessage | |
| if err := json.Unmarshal([]byte(content), &payload); err == nil && payload != nil { |
There was a problem hiding this comment.
The prompt already includes few-shot examples and the candidate relocation path retries invalid responses up to two times, but it currently only parses raw JSON and does not handle the common Markdown fenced JSON form.
I’ll keep the strict JSON contract, but add a small normalization step to unwrap Markdown code fences before parsing.
| func isPositiveInteger(s string) bool { | ||
| for i, r := range s { | ||
| if r < '0' || r > '9' { | ||
| return false | ||
| } | ||
| if i == 0 && r == '0' { | ||
| return false | ||
| } | ||
| } | ||
| return s != "" | ||
| } |
There was a problem hiding this comment.
Bug: isPositiveInteger has a logic error. When the first character is '0', it returns false immediately, which correctly rejects "0" and leading-zero numbers like "01". However, this also means that the string "0" passes the loop (since '0' is between '0' and '9') but then fails the i == 0 && r == '0' check, returning false. This is actually correct behavior for rejecting "0" as a candidate ID (since IDs start at 1). But the function name is misleading — it's not checking for "positive integer" in the mathematical sense, but rather "positive integer without leading zeros". The current logic works for the use case, but the function name could be clearer, or a comment should explain the leading-zero rejection.
Suggestion:
| func isPositiveInteger(s string) bool { | |
| for i, r := range s { | |
| if r < '0' || r > '9' { | |
| return false | |
| } | |
| if i == 0 && r == '0' { | |
| return false | |
| } | |
| } | |
| return s != "" | |
| } | |
| // isPositiveInteger checks if s is a positive integer string (1, 2, 3, ...) | |
| // without leading zeros. Returns false for "0", "01", negative numbers, etc. | |
| func isPositiveInteger(s string) bool { | |
| if s == "" { | |
| return false | |
| } | |
| for i, r := range s { | |
| if r < '0' || r > '9' { | |
| return false | |
| } | |
| // Reject leading zeros (including "0" itself) | |
| if i == 0 && r == '0' { | |
| return false | |
| } | |
| } | |
| return true | |
| } |
| if d != nil { | ||
| if !located && len(candidates) == 0 && r.deps.Template.ReLocationTask != nil { |
There was a problem hiding this comment.
Fallback gap when multiple candidates exist but disambiguation fails or is skipped.
When len(candidates) > 1 and either (a) CandidateReLocationTask is nil, (b) the prompt exceeds the token budget, or (c) the LLM call fails, the comment remains unlocated. The ReLocationTask (snippet-based) fallback below requires len(candidates) == 0, so it is never reached.
In the old code, the ReLocationTask was the fallback for any unlocated comment regardless of how many matches existed. The new condition len(candidates) == 0 creates a gap: comments with multiple ambiguous candidates that can't be disambiguated get no fallback attempt.
Consider relaxing the fallback condition to !located (without the len(candidates) == 0 guard), or at least also trying ReLocationTask when len(candidates) > 1 && !located.
Suggestion:
| if d != nil { | |
| if !located && len(candidates) == 0 && r.deps.Template.ReLocationTask != nil { | |
| if d != nil { | |
| if !located && r.deps.Template.ReLocationTask != nil { |
There was a problem hiding this comment.
When multiple candidates already match the same existing_code, falling back to the old snippet-based RE_LOCATION_TASK can reintroduce the original bug: the regenerated snippet may still be ambiguous and get resolved to the first matching location.
So this PR keeps that path fail-closed:
- 0 candidates: use the existing RE_LOCATION_TASK.
- 1 candidate: apply directly.
- multiple candidates: use CANDIDATE_RE_LOCATION_TASK.
- if candidate selection fails, leave it unlocated instead of guessing.
83bf850 to
d8c61f0
Compare
1 participant
Description
This PR fixes the issue where duplicate
existing_codewithin the same file causes comments to be anchored to the first matching entry. For relevant background and examples, please refer to issue #991.The fix works by first collecting candidates and then deciding whether to perform positioning: if there is only one candidate, it will still be applied directly; if there are multiple candidates, they will be handed over to the new
CANDIDATE_RE_LOCATION_TASKto select acandidate_id; if a candidate cannot be reliably selected, the comment will remain in the unpositioned0-0state instead of guessing the first matching position.Current file candidates still take priority; cross-file candidate selection will only be entered when there are no candidates for the current file. Corresponding protection has also been added in the final output stage to prevent ambiguous comments from being re-parsed to the first matching item.
Limitation
The main goal of this PR is to fix the matching ambiguity of duplicate
existing_codewithin the same file, while reusing the same candidate selection mechanism to handle the cross-file multi-candidate scenario when there are no candidates in the current file. It does not change the scan behavior, timeout behavior, prompt candidate budget, nor does it modify the larger relocation process.Type of Change
How Has This Been Tested?
make testpasses locallyLocal verification passed:
make testgo vet ./...make license-checkmake english-checkgit diff --cached --checkAdded regression test coverage:
existing_codein both the hunk and the full file content will not be directly parsed to the first matching position;code_commentwill triggerCANDIDATE_RE_LOCATION_TASK;candidate_idonly accepts strict JSON, and rejects plain text, bare numbers, fenced JSON, missing fields, and the string"null".Checklist
go fmt,go vet)Related Issues
Fixes #991
Open Question
I would like to ask core developers to confirm a frontend display issue. Currently, the viewer displays the LLM's output but not the input. For the newly added candidate relocation process, the model intentionally outputs only strict JSON, such as
{"candidate_id": 2}. Without seeing the input candidate list, users may not know which code position the2corresponds to.There are two possible approaches here:
candidate_idback to the corresponding code context.