diff --git a/examples/hashline_anchors/README.md b/examples/hashline_anchors/README.md new file mode 100644 index 000000000..bfb6812d6 --- /dev/null +++ b/examples/hashline_anchors/README.md @@ -0,0 +1,38 @@ +# Hashline anchor comment localization (experimental) + +Renders the review diff with per-line anchors (`LINE#HASH:`) and lets the +model localize `code_comment` calls by copying an anchor instead of quoting +`existing_code`. The anchor's hash is verified against the new file content +(two-factor: line number = primary key, hash = checksum, existing_code = +text hint), eliminating the first-match ambiguity of pure text matching. + +Adapted from the hashline protocol (github.com/RimuruW/pi-hashline-edit). + +## Usage + +```bash +OCR_HASHLINE_ANCHORS=1 opencodereview review \ + --from --to \ + --tools examples/hashline_anchors/tools.json +``` + +- `OCR_HASHLINE_ANCHORS=1` — annotate the diff shown to the model with anchors. +- `--tools examples/hashline_anchors/tools.json` — code_comment schema with the + `anchor` parameter and matching description. + +Comments resolved via a verified anchor report `loc_method: "anchor"` in JSON +output; a hash mismatch falls back to the existing text-matching pipeline +(`hunk` / `file` / `relocation`), so behavior is never worse than baseline. + +## Measured effect (offline replay over real commits, production resolver) + +| Localization | opencode repo (95k added lines) | this repo (23k added lines) | +|---|---|---| +| existing_code, 1 line | 63.2% correct | 74.4% correct | +| existing_code, 3 lines | 81.8% correct | 93.0% correct | +| hashline anchor | 100% correct | 100% correct | + +Anchor false-accept rate (wrong line number still passing hash verification): +~0.5%. Diff token overhead of annotation: +26% on the diff itself; in +end-to-end runs total input tokens dropped ~25% as the model needed fewer +file_read round-trips to confirm positions. diff --git a/examples/hashline_anchors/tools.json b/examples/hashline_anchors/tools.json new file mode 100644 index 000000000..3ccbfb683 --- /dev/null +++ b/examples/hashline_anchors/tools.json @@ -0,0 +1,214 @@ +[ + { + "name": "task_done", + "plan_task": false, + "main_task": true, + "definition": { + "name": "task_done", + "description": "Call this tool to terminate task execution when you have completed the user's task, such as when no obvious code issues are found during code review.", + "parameters": { + "type": "object", + "properties": { + "state": { + "type": "string", + "enum": [ + "DONE", + "FAILED" + ], + "description": "Defaults to DONE. Return FAILED if the task cannot be completed using available tools." + } + }, + "required": [ + "state" + ] + } + } + }, + { + "name": "code_comment", + "plan_task": false, + "main_task": true, + "definition": { + "name": "code_comment", + "description": "When you discover that a code change could introduce a code issue, use this tool to report it. The tool pinpoints your feedback to the precise code line (or block) in the current file.\n\n**Core Mechanism (anchor-based):**\nThe diff you were given renders every new-file line with a leading anchor of the form `LINE#HASH:` (e.g. `42#KT:+ some code`). To locate a comment, copy the anchor(s) of the line(s) it applies to into the 'anchor' parameter: a single anchor `42#KT` for one line, or a range `42#KT-45#MQ` for a block. Copy anchors EXACTLY as shown \u2014 the hash is verified. Only anchor lines that are part of the change (added lines). Additionally provide 'existing_code' with the first line of the anchored code as a cross-check.", + "parameters": { + "type": "object", + "properties": { + "comments": { + "type": "array", + "description": "A list of comments. Each item should contain 'content' and 'existing_code'.", + "items": { + "type": "object", + "properties": { + "content": { + "type": "string", + "description": "Comment content, typically a brief description of code issues and corresponding suggestions." + }, + "anchor": { + "type": "string", + "description": "Line anchor(s) copied verbatim from the diff, without the trailing colon or code. Single line: '42#KT'. Block: '42#KT-45#MQ' (start and end anchors of the block)." + }, + "existing_code": { + "type": "string", + "description": "The first line of code at the anchor position, copied exactly from the diff (without the anchor prefix and without the +/- marker). Used as a cross-check for the anchor." + }, + "suggestion_code": { + "type": "string", + "description": "Corresponding suggested code snippet, maintaining consistent code style." + }, + "category": { + "type": "string", + "enum": [ + "bug", + "security", + "performance", + "maintainability", + "test", + "style", + "documentation", + "other" + ], + "description": "The category the issue belongs to." + }, + "severity": { + "type": "string", + "enum": [ + "critical", + "high", + "medium", + "low" + ], + "description": "The severity of the issue." + } + }, + "required": [ + "content", + "anchor", + "existing_code", + "category", + "severity" + ] + } + } + }, + "required": [ + "comments" + ] + } + } + }, + { + "name": "file_read", + "plan_task": false, + "main_task": true, + "definition": { + "name": "file_read", + "description": "Use this tool to read file content when you need to get context for git diff. You can specify start_line and end_line to view specific parts of the file.\n\n**Line Range Strategy:**\n- Git diff hunk header provides guidance on how to get more relevant context.\n- Git diff hunk header \"@@-x,y +m,n@@\" indicates that the old file has y lines starting from line x, and the new file has n lines starting from line m.\n- For example, when you need to read 50 lines above and below the current changed code block in the new file, set start_line = m - 50, end_line = m + n + 50.\n\n**Example output:**\nFile\uff1apath/to/example.go (Total lines: 50)\nIS_TRUNCATED: false\nLINE_RANGE: 10-12\n// The following is the original content of the file\nfunc main() {\n fmt.Println(\"Hello, World!\")\n}\n\n**Limitations:**\n- If the specified range exceeds 500 lines, only 500 lines will be returned with a truncation notice.\n- This tool can only read file content from the modified version (after changes) in git diff.", + "parameters": { + "type": "object", + "properties": { + "file_path": { + "type": "string", + "description": "The relative path of the file to open." + }, + "start_line": { + "type": "integer", + "description": "The start line number to view. Defaults to 1." + }, + "end_line": { + "type": "integer", + "description": "The end line number to view. Defaults to end line of file." + } + }, + "required": [ + "file_path" + ] + } + } + }, + { + "name": "code_search", + "plan_task": true, + "main_task": true, + "definition": { + "name": "code_search", + "description": "Use this tool to search for specific text within files. Supports searching in specific files, directories, or across the entire codebase with flexible file pattern filtering. Can use either exact string matching or regular expressions.\n\n**Example output:**\nSearch results for 'toolRequest' (case-insensitive):\nFile: path/to/example.java\n433| String name = toolRequest.get().getName();\n438| logToolRequest(newPath, tool, toolRequest.get());\n\n**Regular expression examples (requires use_perl_regexp: true):**\n- Find classes that extend BaseModel: 'class.*extends.*BaseModel'\n- Find function: 'functionName(.*)'\n- Find the function call sites: '\\.functionName(.*)'\n- Match any of multiple strings: 'error|exception|fail'\n\n**File patterns examples:**\n- Single file: ['src/main.go']\n- Multiple files: ['src/main.go', 'lib/utils.js']\n- All Go files: ['*.go']\n- Exclude test files: [':(exclude)*_test.go']\n- Only in src directory: ['src/']\n- Multiple patterns: ['*.go', ':(exclude)vendor/']\n\n**Limitations:**\n- If more than 100 matches are found, only the first 100 results will be returned.\n- Empty search terms will return no results.\n- This tool searches in the current version of files.", + "parameters": { + "type": "object", + "properties": { + "search_text": { + "type": "string", + "description": "The text string or regular expression pattern to search for." + }, + "file_patterns": { + "type": "array", + "items": { + "type": "string" + }, + "description": "Array of patterns to include/exclude files in the search. Supports Git pathspec syntax for including and excluding files. If omitted, searches the entire codebase." + }, + "case_sensitive": { + "type": "boolean", + "description": "Whether the search should be case-sensitive. Defaults to false (case-insensitive)." + }, + "use_perl_regexp": { + "type": "boolean", + "description": "If true, treats search_text as a Perl-compatible regular expression pattern instead of literal text. Defaults to false." + } + }, + "required": [ + "search_text" + ] + } + } + }, + { + "name": "file_read_diff", + "plan_task": true, + "main_task": true, + "definition": { + "name": "file_read_diff", + "description": "The tool is used to view the changes made to other files in the list of modifications. Call this tool when you discover suspected code issues but need to check changes in other files to confirm whether the problem actually exists. This tool will respond in git diff format.\n\nOutput example:\n==== FILE: path/to/file1.txt ====\n--- a/path/to/file1.txt\n+++ b/path/to/file1.txt\n@@ -10,1 +10,1 @@\n- old content\n+ new content\n\n==== FILE: path/to/file2.txt ====\n@@ -5,1 +5,2 @@\n - old content\n + new content1\n + new content2", + "parameters": { + "type": "object", + "properties": { + "path_array": { + "type": "array", + "items": { + "type": "string" + }, + "description": "List of file paths to view diff content." + } + }, + "required": [ + "path_array" + ] + } + } + }, + { + "name": "file_find", + "plan_task": true, + "main_task": true, + "definition": { + "name": "file_find", + "description": "Search for matching files in the current project based on filename keywords. Use this tool when you cannot find the files you need to view in the current change file list.\n\nThis tool searches for filenames containing specified keywords in the project directory and returns a list of matching file paths. Search is case-insensitive by default, adjustable via case_sensitive parameter.\n\nNote: This tool only supports returning the first 100 matching file paths; excess will be truncated.\n\nExample:\nInput:\nquery_name: UserService\nOutput:\nsrc/main/java/UserService.java\nsrc/test/java/UserServiceTest.java", + "parameters": { + "type": "object", + "properties": { + "query_name": { + "type": "string", + "description": "Filename keyword to search for, supports partial matching." + }, + "case_sensitive": { + "type": "boolean", + "description": "Whether to perform case-sensitive search. Defaults to false." + } + }, + "required": [ + "query_name" + ] + } + } + } +] \ No newline at end of file diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 00367acf7..4f973ffdb 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -21,6 +21,7 @@ import ( "github.com/alibaba/open-code-review/internal/config/toolsconfig" "github.com/alibaba/open-code-review/internal/diff" "github.com/alibaba/open-code-review/internal/gitcmd" + "github.com/alibaba/open-code-review/internal/hashline" "github.com/alibaba/open-code-review/internal/llm" "github.com/alibaba/open-code-review/internal/llmloop" "github.com/alibaba/open-code-review/internal/model" @@ -469,7 +470,11 @@ func (a *Agent) injectDiffMap() { for i := range a.diffs { d := &a.diffs[i] if d.NewPath != "/dev/null" { - m[d.NewPath] = d.Diff + if hashlineAnchorsEnabled() { + m[d.NewPath] = hashline.AnnotateDiff(d) + } else { + m[d.NewPath] = d.Diff + } } } dm := tool.NewDiffMap(m) @@ -1136,13 +1141,17 @@ func (a *Agent) executeSubtask(ctx context.Context, d model.Diff) (bool, *subtas rawMsgs := a.args.Template.MainTask.Messages messages := make([]llm.Message, 0, len(rawMsgs)) + diffForPrompt := d.Diff + if hashlineAnchorsEnabled() { + diffForPrompt = hashline.AnnotateDiff(&d) + } for _, m := range rawMsgs { content := m.Content content = strings.ReplaceAll(content, "{{current_system_date_time}}", a.currentDate) content = strings.ReplaceAll(content, "{{current_file_path}}", newPath) content = strings.ReplaceAll(content, "{{system_rule}}", rule) content = strings.ReplaceAll(content, "{{change_files}}", changeFilesExcludingCurrent) - content = strings.ReplaceAll(content, "{{diff}}", d.Diff) + content = strings.ReplaceAll(content, "{{diff}}", diffForPrompt) content = strings.ReplaceAll(content, "{{requirement_background}}", a.args.Background) // Always substitute the {{plan_guidance}} token so the literal placeholder // never leaks into the rendered prompt. When the plan phase produced no diff --git a/internal/agent/hashline_mode.go b/internal/agent/hashline_mode.go new file mode 100644 index 000000000..c0d5254d7 --- /dev/null +++ b/internal/agent/hashline_mode.go @@ -0,0 +1,15 @@ +package agent + +import "os" + +// hashlineAnchorsEnabled reports whether hashline anchor mode is on. +// Set OCR_HASHLINE_ANCHORS=1 to render the main-task diff with per-line +// "LINE#HASH:" anchors and let the model localize comments via the +// code_comment "anchor" field instead of (or in addition to) existing_code. +func hashlineAnchorsEnabled() bool { + switch os.Getenv("OCR_HASHLINE_ANCHORS") { + case "1", "true", "on", "yes": + return true + } + return false +} diff --git a/internal/diff/relocation.go b/internal/diff/relocation.go index d5a933219..d161eb6f9 100644 --- a/internal/diff/relocation.go +++ b/internal/diff/relocation.go @@ -68,6 +68,7 @@ func ReLocateComment( original := cm.ExistingCode cm.ExistingCode = code if ResolveComment(cm, d) { + cm.LocMethod = "relocation" return true, resp, messages } cm.ExistingCode = original diff --git a/internal/diff/resolver.go b/internal/diff/resolver.go index d60dfeaa9..a763109e6 100644 --- a/internal/diff/resolver.go +++ b/internal/diff/resolver.go @@ -3,6 +3,7 @@ package diff import ( "strings" + "github.com/alibaba/open-code-review/internal/hashline" "github.com/alibaba/open-code-review/internal/model" ) @@ -42,6 +43,11 @@ func ResolveLineNumbers(comments []model.LlmComment, diffs []model.Diff) []model continue } + // Fast path: verified hashline anchor (O(1), unambiguous). + if resolveFromAnchor(d, cm) { + continue + } + // Primary: try matching from deleted/context lines in diff hunks if resolveFromHunk(d, cm) { continue @@ -60,6 +66,9 @@ func ResolveComment(cm *model.LlmComment, d *model.Diff) bool { if cm.StartLine > 0 || cm.EndLine > 0 { return true } + if resolveFromAnchor(d, cm) { + return true + } if cm.ExistingCode == "" { return false } @@ -69,6 +78,57 @@ func ResolveComment(cm *model.LlmComment, d *model.Diff) bool { return resolveFromFileContent(d, cm) } +// resolveFromAnchor resolves the comment position from a hashline anchor +// ("12#KT" or "12#KT-18#MQ") verified against the new file content. +// +// Two-factor validation, following the hashline protocol: +// - The hash is the checksum: both endpoint anchors must verify against the +// current 3-line context window in NewFileContent. +// - ExistingCode, when present, acts as a text hint that can veto a +// hash collision: the first non-blank line of existing_code must appear +// somewhere within the anchored range (normalized comparison). +func resolveFromAnchor(d *model.Diff, cm *model.LlmComment) bool { + if cm.Anchor == "" || d.NewFileContent == "" { + return false + } + start, end, ok := hashline.ResolveSpec(cm.Anchor, d.NewFileContent) + if !ok { + return false + } + + // textHint veto: if existing_code is present, its first significant line + // must be found inside the anchored range. + if hint := firstSignificantLine(cm.ExistingCode); hint != "" { + fileLines := strings.Split(d.NewFileContent, "\n") + found := false + for ln := start; ln <= end && ln <= len(fileLines); ln++ { + if normalizeLine(fileLines[ln-1]) == hint { + found = true + break + } + } + if !found { + cm.LocMethod = "anchor_hint_veto" + return false + } + } + + cm.StartLine = start + cm.EndLine = end + cm.LocMethod = "anchor" + return true +} + +// firstSignificantLine returns the first normalized non-blank line of code. +func firstSignificantLine(code string) string { + for _, line := range strings.Split(code, "\n") { + if n := normalizeLine(line); n != "" { + return n + } + } + return "" +} + // indexedLine pairs a normalized line with its absolute file line number. type indexedLine struct { lineNum int @@ -95,6 +155,7 @@ func resolveFromHunk(d *model.Diff, cm *model.LlmComment) bool { if start, end, ok := matchConsecutive(newSide, targetLines); ok { cm.StartLine = start cm.EndLine = end + cm.LocMethod = "hunk" return true } } @@ -104,6 +165,7 @@ func resolveFromHunk(d *model.Diff, cm *model.LlmComment) bool { if start, end, ok := matchConsecutive(oldSide, targetLines); ok { cm.StartLine = start cm.EndLine = end + cm.LocMethod = "hunk" return true } } @@ -206,6 +268,7 @@ func resolveFromFileContent(d *model.Diff, cm *model.LlmComment) bool { if matched { cm.StartLine = fileLineNums[i] cm.EndLine = fileLineNums[i+len(targetLines)-1] + cm.LocMethod = "file" return true } } diff --git a/internal/diff/resolver_anchor_test.go b/internal/diff/resolver_anchor_test.go new file mode 100644 index 000000000..0b6a69804 --- /dev/null +++ b/internal/diff/resolver_anchor_test.go @@ -0,0 +1,66 @@ +package diff + +import ( + "strings" + "testing" + + "github.com/alibaba/open-code-review/internal/hashline" + "github.com/alibaba/open-code-review/internal/model" +) + +func anchorFor(content string, line int) string { + lines := strings.Split(content, "\n") + return hashline.FormatAnchor(lines, line-1) +} + +func TestResolveComment_AnchorFastPath(t *testing.T) { + content := "a\nb\nrepeated\nc\nrepeated\nd\n" + d := &model.Diff{NewPath: "f.go", NewFileContent: content, Diff: "@@ -1,6 +1,6 @@\n a\n b\n+repeated\n c\n repeated\n d"} + + // Anchor points at the SECOND "repeated" (line 5) — text matching would + // always pick the first occurrence (line 3). + cm := &model.LlmComment{Path: "f.go", Content: "x", ExistingCode: "repeated", Anchor: anchorFor(content, 5)} + if !ResolveComment(cm, d) { + t.Fatal("anchor resolution failed") + } + if cm.StartLine != 5 || cm.EndLine != 5 || cm.LocMethod != "anchor" { + t.Fatalf("got (%d,%d,%s), want (5,5,anchor)", cm.StartLine, cm.EndLine, cm.LocMethod) + } + + // Same comment without anchor: sliding window picks line 3 (ambiguity bug). + cm2 := &model.LlmComment{Path: "f.go", Content: "x", ExistingCode: "repeated"} + if !ResolveComment(cm2, d) { + t.Fatal("text resolution failed") + } + if cm2.StartLine != 3 { + t.Fatalf("expected legacy path to pick first occurrence (3), got %d", cm2.StartLine) + } +} + +func TestResolveComment_AnchorHintVeto(t *testing.T) { + content := "alpha\nbeta\ngamma\n" + d := &model.Diff{NewPath: "f.go", NewFileContent: content, Diff: "@@ -1,3 +1,3 @@\n alpha\n+beta\n gamma"} + + // Valid anchor for line 2 but existing_code claims totally different code: + // the hint veto must reject the anchor, then text fallback also fails. + cm := &model.LlmComment{Path: "f.go", Content: "x", ExistingCode: "does_not_exist()", Anchor: anchorFor(content, 2)} + if ResolveComment(cm, d) { + t.Fatalf("expected veto+fallback failure, got (%d,%d,%s)", cm.StartLine, cm.EndLine, cm.LocMethod) + } + if cm.LocMethod != "anchor_hint_veto" { + t.Fatalf("LocMethod = %q, want anchor_hint_veto", cm.LocMethod) + } +} + +func TestResolveComment_BadAnchorFallsBackToText(t *testing.T) { + content := "alpha\nbeta\ngamma\n" + d := &model.Diff{NewPath: "f.go", NewFileContent: content, Diff: "@@ -1,3 +1,3 @@\n alpha\n+beta\n gamma"} + + cm := &model.LlmComment{Path: "f.go", Content: "x", ExistingCode: "beta", Anchor: "2#ZZ"} // wrong hash + if !ResolveComment(cm, d) { + t.Fatal("expected fallback text resolution to succeed") + } + if cm.StartLine != 2 || cm.LocMethod != "hunk" { + t.Fatalf("got (%d,%s), want (2,hunk)", cm.StartLine, cm.LocMethod) + } +} diff --git a/internal/hashline/annotate.go b/internal/hashline/annotate.go new file mode 100644 index 000000000..b421e1bd1 --- /dev/null +++ b/internal/hashline/annotate.go @@ -0,0 +1,113 @@ +package hashline + +import ( + "strconv" + "strings" + + "github.com/alibaba/open-code-review/internal/model" +) + +// AnnotateDiff renders the unified diff of d with hashline anchors on every +// line that exists in the new file (added and context lines). Deleted lines +// and hunk/file headers are passed through unchanged. +// +// Anchor hashes are computed against d.NewFileContent (the authoritative +// post-change file), so an anchor copied from the annotated diff verifies +// against the same file content used by ResolveSpec. +// +// Output line format for new-side lines: +// +// 12#KT:+added line content +// 13#MQ: context line content +// +// If NewFileContent is empty or a hunk's new-side line numbers fall outside +// the file (malformed diff), the original diff text is returned unmodified. +func AnnotateDiff(d *model.Diff) string { + if d == nil || d.Diff == "" || d.NewFileContent == "" { + if d == nil { + return "" + } + return d.Diff + } + newLines := strings.Split(d.NewFileContent, "\n") + + var out strings.Builder + lines := strings.Split(d.Diff, "\n") + newLine := 0 // 1-based new-file line of the *next* new-side hunk line + inHunk := false + + for i, line := range lines { + if i > 0 { + out.WriteByte('\n') + } + if m := hunkHeaderRe.FindStringSubmatch(line); m != nil { + newLine, _ = strconv.Atoi(m[1]) + inHunk = true + out.WriteString(line) + continue + } + if !inHunk || line == "" || strings.HasPrefix(line, "\\") || + strings.HasPrefix(line, "diff --git ") { + if strings.HasPrefix(line, "diff --git ") { + inHunk = false + } + out.WriteString(line) + continue + } + switch line[0] { + case '+', ' ': + idx := newLine - 1 + // Only annotate when the diff line content actually matches the + // new file content at that position; otherwise pass through + // (defensive against malformed diffs / stale file content). + if idx >= 0 && idx < len(newLines) && + NormalizeHashInput(line[1:]) == NormalizeHashInput(newLines[idx]) { + out.WriteString(FormatAnchor(newLines, idx)) + out.WriteByte(':') + } + out.WriteString(line) + newLine++ + case '-': + out.WriteString(line) + default: + // Header-ish line inside hunk region (shouldn't happen) — pass through. + out.WriteString(line) + } + } + return out.String() +} + +var hunkHeaderRe = mustHunkRe() + +func mustHunkRe() *hunkRe { return &hunkRe{} } + +// hunkRe is a tiny allocation-free matcher for "@@ -a[,b] +c[,d] @@" headers +// that extracts the new-file start line. Using a hand-rolled matcher avoids a +// regexp dependency in the hot path. +type hunkRe struct{} + +// FindStringSubmatch mimics regexp: returns nil or [full, newStart]. +func (h *hunkRe) FindStringSubmatch(line string) []string { + if !strings.HasPrefix(line, "@@ -") { + return nil + } + plus := strings.Index(line, " +") + if plus < 0 { + return nil + } + rest := line[plus+2:] + end := strings.IndexAny(rest, ", @") + if end < 0 { + return nil + } + numStr := rest[:end] + if numStr == "" { + return nil + } + for _, c := range numStr { + if c < '0' || c > '9' { + return nil + } + } + return []string{line, numStr} +} diff --git a/internal/hashline/hashline.go b/internal/hashline/hashline.go new file mode 100644 index 000000000..19387d830 --- /dev/null +++ b/internal/hashline/hashline.go @@ -0,0 +1,185 @@ +// Package hashline implements content-hash line anchors for comment +// localization, adapted from the hashline protocol +// (github.com/RimuruW/pi-hashline-edit, itself adapted from oh-my-pi). +// +// Every line of a file gets a short hash computed from the line and its +// immediate neighbors (prev + "\0" + curr + "\0" + next). An anchor +// "LINE#HASH" therefore carries both a position (line number, the primary +// key) and a checksum (the hash, a verification factor). Identical lines in +// different contexts get different hashes, so anchors are unambiguous even +// for repeated code. +package hashline + +import ( + "regexp" + "strconv" + "strings" +) + +// HashLen is the number of hash characters per anchor (one nibble each). +const HashLen = 2 + +// NibbleStr is the 16-character hash alphabet from pi-hashline-edit. +// It excludes most hex digits, digit-lookalikes and vowels. +const NibbleStr = "ZPMQVRWSNKTXJBYH" + +// anchorRe matches "12#KT" or a range "12#KT-18#MQ". +var anchorRe = regexp.MustCompile( + `^\s*(\d+)#([` + NibbleStr + `]{2,4})(?:\s*-\s*(\d+)#([` + NibbleStr + `]{2,4}))?\s*$`) + +// ─── xxh32 ────────────────────────────────────────────────────────────── + +const ( + prime1 uint32 = 2654435761 + prime2 uint32 = 2246822519 + prime3 uint32 = 3266489917 + prime4 uint32 = 668265263 + prime5 uint32 = 374761393 +) + +func rotl32(x uint32, r uint) uint32 { return x<>(32-r) } + +func round32(acc, input uint32) uint32 { + acc += input * prime2 + acc = rotl32(acc, 13) + acc *= prime1 + return acc +} + +func le32(b []byte) uint32 { + return uint32(b[0]) | uint32(b[1])<<8 | uint32(b[2])<<16 | uint32(b[3])<<24 +} + +// XXH32 computes the xxHash32 of input with the given seed. +func XXH32(input []byte, seed uint32) uint32 { + n := len(input) + var h uint32 + i := 0 + if n >= 16 { + v1 := seed + prime1 + prime2 + v2 := seed + prime2 + v3 := seed + v4 := seed - prime1 + for ; i <= n-16; i += 16 { + v1 = round32(v1, le32(input[i:])) + v2 = round32(v2, le32(input[i+4:])) + v3 = round32(v3, le32(input[i+8:])) + v4 = round32(v4, le32(input[i+12:])) + } + h = rotl32(v1, 1) + rotl32(v2, 7) + rotl32(v3, 12) + rotl32(v4, 18) + } else { + h = seed + prime5 + } + h += uint32(n) + for ; i <= n-4; i += 4 { + h += le32(input[i:]) * prime3 + h = rotl32(h, 17) * prime4 + } + for ; i < n; i++ { + h += uint32(input[i]) * prime5 + h = rotl32(h, 11) * prime1 + } + h ^= h >> 15 + h *= prime2 + h ^= h >> 13 + h *= prime3 + h ^= h >> 16 + return h +} + +// ─── Line hashing ─────────────────────────────────────────────────────── + +// NormalizeHashInput normalizes a line before hashing: strip \r, trim right. +func NormalizeHashInput(line string) string { + return strings.TrimRight(strings.ReplaceAll(line, "\r", ""), " \t\n\v\f") +} + +// ComputeHashFromContext computes a HashLen-char hash from a line and its +// normalized neighbors. Neighbors outside the file use "". +func ComputeHashFromContext(prev, curr, next string) string { + h := XXH32([]byte(prev+"\x00"+curr+"\x00"+next), 0) + var b strings.Builder + for i := HashLen - 1; i >= 0; i-- { + b.WriteByte(NibbleStr[(h>>(uint(i)*4))&0x0f]) + } + return b.String() +} + +// ComputeLineHash computes the anchor hash for fileLines[index] (0-based). +func ComputeLineHash(fileLines []string, index int) string { + prev, next := "", "" + if index > 0 { + prev = NormalizeHashInput(fileLines[index-1]) + } + if index < len(fileLines)-1 { + next = NormalizeHashInput(fileLines[index+1]) + } + return ComputeHashFromContext(prev, NormalizeHashInput(fileLines[index]), next) +} + +// FormatAnchor renders "LINE#HASH" for fileLines[index] (0-based index, +// 1-based rendered line number). +func FormatAnchor(fileLines []string, index int) string { + return strconv.Itoa(index+1) + "#" + ComputeLineHash(fileLines, index) +} + +// ─── Anchor parsing & resolution ──────────────────────────────────────── + +// Anchor is a parsed LINE#HASH reference. +type Anchor struct { + Line int // 1-based + Hash string +} + +// ParseSpec parses "12#KT" or "12#KT-18#MQ" into start/end anchors. +// For the single form, end == start. +func ParseSpec(spec string) (start, end Anchor, ok bool) { + m := anchorRe.FindStringSubmatch(spec) + if m == nil { + return Anchor{}, Anchor{}, false + } + sl, err := strconv.Atoi(m[1]) + if err != nil || sl <= 0 { + return Anchor{}, Anchor{}, false + } + start = Anchor{Line: sl, Hash: m[2]} + if m[3] == "" { + return start, start, true + } + el, err := strconv.Atoi(m[3]) + if err != nil || el <= 0 { + return Anchor{}, Anchor{}, false + } + end = Anchor{Line: el, Hash: m[4]} + return start, end, true +} + +// Verify reports whether anchor a matches the content of fileLines. +func Verify(fileLines []string, a Anchor) bool { + idx := a.Line - 1 + if idx < 0 || idx >= len(fileLines) { + return false + } + return ComputeLineHash(fileLines, idx) == a.Hash +} + +// ResolveSpec parses an anchor spec and verifies both endpoints against the +// file content. On success it returns the resolved 1-based [start, end] range. +// Strict by design: a hash mismatch returns ok=false — no fuzzy relocation. +func ResolveSpec(spec string, fileContent string) (startLine, endLine int, ok bool) { + if spec == "" || fileContent == "" { + return 0, 0, false + } + start, end, ok := ParseSpec(spec) + if !ok { + return 0, 0, false + } + if end.Line < start.Line { + start, end = end, start + } + lines := strings.Split(fileContent, "\n") + if !Verify(lines, start) || !Verify(lines, end) { + return 0, 0, false + } + return start.Line, end.Line, true +} diff --git a/internal/hashline/hashline_test.go b/internal/hashline/hashline_test.go new file mode 100644 index 000000000..f3a0e87eb --- /dev/null +++ b/internal/hashline/hashline_test.go @@ -0,0 +1,104 @@ +package hashline + +import ( + "strings" + "testing" + + "github.com/alibaba/open-code-review/internal/model" +) + +func TestAnchorRoundTrip(t *testing.T) { + file := "package main\n\nfunc main() {\n\tx := 1\n\tx := 1\n\tprintln(x)\n}\n" + lines := strings.Split(file, "\n") + for i := range lines { + spec := FormatAnchor(lines, i) + s, e, ok := ResolveSpec(spec, file) + if !ok || s != i+1 || e != i+1 { + t.Fatalf("line %d: spec %q resolved to (%d,%d,%v)", i+1, spec, s, e, ok) + } + } +} + +func TestIdenticalLinesGetDifferentContextHashes(t *testing.T) { + // Lines 4 and 5 are identical ("\tx := 1") but have different neighbors. + lines := []string{"a", "x := 1", "b", "c", "x := 1", "d"} + h1 := ComputeLineHash(lines, 1) + h2 := ComputeLineHash(lines, 4) + if h1 == h2 { + t.Fatalf("expected different hashes for identical lines in different contexts, both %q", h1) + } +} + +func TestWrongHashRejected(t *testing.T) { + file := "one\ntwo\nthree\n" + lines := strings.Split(file, "\n") + good := FormatAnchor(lines, 1) // "2#XX" + // Corrupt the hash deterministically: swap to a different valid pair. + bad := good[:len(good)-2] + if strings.HasSuffix(good, "ZZ") { + bad += "PP" + } else { + bad += "ZZ" + } + if _, _, ok := ResolveSpec(bad, file); ok { + t.Fatalf("corrupted anchor %q (from %q) should not resolve", bad, good) + } +} + +func TestRangeSpec(t *testing.T) { + file := "alpha\nbeta\ngamma\ndelta\n" + lines := strings.Split(file, "\n") + spec := FormatAnchor(lines, 1) + "-" + FormatAnchor(lines, 3) + s, e, ok := ResolveSpec(spec, file) + if !ok || s != 2 || e != 4 { + t.Fatalf("range spec %q => (%d,%d,%v), want (2,4,true)", spec, s, e, ok) + } +} + +func TestAnnotateDiff(t *testing.T) { + newContent := "line one\nline two changed\nline three\nline four added\nline five\n" + diffText := "@@ -1,4 +1,5 @@\n line one\n-line two\n+line two changed\n line three\n+line four added\n line five" + d := &model.Diff{Diff: diffText, NewFileContent: newContent} + out := AnnotateDiff(d) + lines := strings.Split(out, "\n") + newLines := strings.Split(newContent, "\n") + + wantPrefix := map[int]string{ // diff line idx -> expected anchor prefix + 1: FormatAnchor(newLines, 0) + ": line one", + 3: FormatAnchor(newLines, 1) + ":+line two changed", + 4: FormatAnchor(newLines, 2) + ": line three", + 5: FormatAnchor(newLines, 3) + ":+line four added", + 6: FormatAnchor(newLines, 4) + ": line five", + } + if lines[0] != "@@ -1,4 +1,5 @@" { + t.Fatalf("hunk header changed: %q", lines[0]) + } + if lines[2] != "-line two" { + t.Fatalf("deleted line should be untouched, got %q", lines[2]) + } + for idx, want := range wantPrefix { + if lines[idx] != want { + t.Errorf("diff line %d = %q, want %q", idx, lines[idx], want) + } + } + + // Every anchor in the annotated diff must resolve against the new file. + for _, l := range lines { + hashPos := strings.Index(l, "#") + colon := strings.Index(l, ":") + if hashPos < 0 || colon < 0 || hashPos > colon { + continue + } + spec := l[:colon] + if _, _, ok := ResolveSpec(spec, newContent); !ok { + t.Errorf("annotated anchor %q does not resolve", spec) + } + } +} + +func TestAnnotateDiffPassthroughWithoutContent(t *testing.T) { + d := &model.Diff{Diff: "@@ -1 +1 @@\n-a\n+b"} + if got := AnnotateDiff(d); got != d.Diff { + t.Fatalf("expected passthrough, got %q", got) + } +} diff --git a/internal/model/review.go b/internal/model/review.go index d75577653..0e6904038 100644 --- a/internal/model/review.go +++ b/internal/model/review.go @@ -6,9 +6,16 @@ type LlmComment struct { Content string `json:"content"` SuggestionCode string `json:"suggestion_code,omitempty"` ExistingCode string `json:"existing_code,omitempty"` - StartLine int `json:"start_line"` - EndLine int `json:"end_line"` - Thinking string `json:"thinking,omitempty"` + // Anchor is an optional hashline anchor ("12#KT" or "12#KT-18#MQ") + // copied by the model from an anchor-annotated diff. When present and + // verified, it resolves the comment position in O(1) without text matching. + Anchor string `json:"anchor,omitempty"` + StartLine int `json:"start_line"` + EndLine int `json:"end_line"` + // LocMethod records how StartLine/EndLine were resolved: + // "anchor", "anchor_hint_veto", "hunk", "file", "relocation", or "" (unresolved). + LocMethod string `json:"loc_method,omitempty"` + Thinking string `json:"thinking,omitempty"` // Category classifies the finding. One of: // bug, security, performance, maintainability, test, style, documentation, other. Category string `json:"category,omitempty"` diff --git a/internal/tool/code_comment.go b/internal/tool/code_comment.go index 53622e697..7f2c9a5c0 100644 --- a/internal/tool/code_comment.go +++ b/internal/tool/code_comment.go @@ -100,6 +100,9 @@ func ParseComments(args map[string]any) ([]model.LlmComment, string) { if existing, ok := obj["existing_code"].(string); ok { cm.ExistingCode = existing } + if anchor, ok := obj["anchor"].(string); ok { + cm.Anchor = strings.TrimSpace(anchor) + } if thinking, ok := obj["thinking"].(string); ok { cm.Thinking = thinking }