Skip to content

Commit af4d30d

Browse files
committed
fix: bugs found during ollama end-to-end testing
1. extractDiff: fall back to plain ``` blocks when model omits the "diff" language tag — matches blocks starting with "diff " or "--- " 2. parseMinimal: skip go compiler package headers (": # testproject") that were being parsed as lint issues and sent to the model 3. normalizeIssuePaths: resolve linter output paths relative to the target dir so "../../../tmp/project/main.go" and "./main.go" merge into a single fix task instead of splitting into two
1 parent ec9ceb1 commit af4d30d

File tree

5 files changed

+121
-8
lines changed

5 files changed

+121
-8
lines changed

pkg/linter/linter.go

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"context"
66
"fmt"
77
"os/exec"
8+
"path/filepath"
89
"regexp"
910
"strconv"
1011
"strings"
@@ -95,11 +96,17 @@ func parseMinimal(line string) (Issue, bool) {
9596
if m == nil {
9697
return Issue{}, false
9798
}
99+
msg := m[3]
100+
// Skip go compiler package headers (e.g. ": # testproject") and
101+
// malformed messages that start with ": " (mangled column parse).
102+
if strings.HasPrefix(msg, ": ") || strings.HasPrefix(msg, "# ") {
103+
return Issue{}, false
104+
}
98105
lineNum, _ := strconv.Atoi(m[2])
99106
return Issue{
100107
File: m[1],
101108
Line: lineNum,
102-
Message: m[3],
109+
Message: msg,
103110
Linter: "custom",
104111
}, true
105112
}
@@ -113,7 +120,33 @@ func RunCommand(ctx context.Context, dir string, cmd []string) (ParseResult, err
113120
return ParseResult{}, fmt.Errorf("running %s: %w", cmd[0], err)
114121
}
115122
}
116-
return ParseOutput(string(out)), nil
123+
result := ParseOutput(string(out))
124+
normalizeIssuePaths(result.Issues, dir)
125+
return result, nil
126+
}
127+
128+
// normalizeIssuePaths resolves parsed file paths relative to dir.
129+
// Linters sometimes emit absolute or CWD-relative paths for compile errors
130+
// (e.g. "../../../tmp/project/main.go") even when run from dir. This
131+
// normalizes them so they're consistent with the target directory.
132+
func normalizeIssuePaths(issues []Issue, dir string) {
133+
absDir, err := filepath.Abs(dir)
134+
if err != nil {
135+
return
136+
}
137+
for i := range issues {
138+
f := issues[i].File
139+
// Resolve the file path against the command's working dir.
140+
absFile := f
141+
if !filepath.IsAbs(f) {
142+
absFile = filepath.Join(absDir, f)
143+
}
144+
absFile = filepath.Clean(absFile)
145+
// Make it relative to dir.
146+
if rel, err := filepath.Rel(absDir, absFile); err == nil {
147+
issues[i].File = rel
148+
}
149+
}
117150
}
118151

119152
func Run(ctx context.Context, dir string) (ParseResult, error) {

pkg/linter/linter_test.go

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,3 +210,34 @@ func TestRun(t *testing.T) {
210210
// Run tries golangci-lint. Whether installed or not, the function itself is exercised.
211211
_, _ = Run(context.Background(), t.TempDir())
212212
}
213+
214+
func TestParseMinimalSkipsCompilerHeaders(t *testing.T) {
215+
// golangci-lint typecheck errors produce lines like:
216+
// ../../../tmp/project/main.go:1: : # testproject
217+
raw := `../../../tmp/project/main.go:1: : # testproject
218+
./main.go:5:2: "os" imported and not used
219+
./main.go:9:2: declared and not used: x`
220+
result := ParseOutput(raw)
221+
// Should parse the 2 real issues but skip the compiler header.
222+
if len(result.Issues) != 2 {
223+
t.Errorf("expected 2 issues, got %d", len(result.Issues))
224+
for _, iss := range result.Issues {
225+
t.Logf(" %s:%d: %s", iss.File, iss.Line, iss.Message)
226+
}
227+
}
228+
}
229+
230+
func TestNormalizeIssuePaths(t *testing.T) {
231+
dir := t.TempDir()
232+
issues := []Issue{
233+
{File: "./main.go", Line: 1, Message: "err"},
234+
{File: "pkg/foo.go", Line: 2, Message: "err"},
235+
}
236+
normalizeIssuePaths(issues, dir)
237+
if issues[0].File != "main.go" {
238+
t.Errorf("expected normalized main.go, got %s", issues[0].File)
239+
}
240+
if issues[1].File != "pkg/foo.go" {
241+
t.Errorf("expected pkg/foo.go unchanged, got %s", issues[1].File)
242+
}
243+
}

pkg/worker/ollama.go

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -129,15 +129,24 @@ func ollamaChat(ctx context.Context, client *http.Client, cfg OllamaConfig, prom
129129
return chatResp.Message.Content, nil
130130
}
131131

132-
var diffBlockRe = regexp.MustCompile("(?s)```diff\\s*\n(.*?)```")
132+
var (
133+
// Matches ```diff\n...\n``` (preferred format).
134+
diffBlockRe = regexp.MustCompile("(?s)```diff\\s*\n(.*?)```")
135+
// Fallback: matches ```\n...\n``` when content starts with diff/--- headers.
136+
plainBlockRe = regexp.MustCompile("(?s)```\\s*\n((?:diff |--- ).*?)```")
137+
)
133138

134-
// extractDiff pulls the first unified diff from ```diff ... ``` markers.
139+
// extractDiff pulls the first unified diff from fenced code blocks.
140+
// Prefers ```diff blocks, falls back to plain ``` blocks whose content
141+
// starts with "diff " or "--- " (common when models omit the language tag).
135142
func extractDiff(response string) string {
136-
m := diffBlockRe.FindStringSubmatch(response)
137-
if len(m) < 2 {
138-
return ""
143+
if m := diffBlockRe.FindStringSubmatch(response); len(m) >= 2 {
144+
return strings.TrimSpace(m[1])
145+
}
146+
if m := plainBlockRe.FindStringSubmatch(response); len(m) >= 2 {
147+
return strings.TrimSpace(m[1])
139148
}
140-
return strings.TrimSpace(m[1])
149+
return ""
141150
}
142151

143152
// applyDiff writes the diff to a temp file and runs `patch -p1` in the target dir.

pkg/worker/ollama_test.go

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,3 +27,42 @@ func TestExtractDiffMultipleBlocks(t *testing.T) {
2727
t.Errorf("expected first diff block, got %q", got)
2828
}
2929
}
30+
31+
func TestExtractDiffPlainBlock(t *testing.T) {
32+
// Model omits "diff" language tag but content starts with "diff " header.
33+
response := "Here:\n\n```\ndiff --git a/main.go b/main.go\n--- a/main.go\n+++ b/main.go\n@@ -1 +1 @@\n-old\n+new\n```"
34+
got := extractDiff(response)
35+
if got == "" {
36+
t.Fatal("expected diff from plain block, got empty")
37+
}
38+
if got != "diff --git a/main.go b/main.go\n--- a/main.go\n+++ b/main.go\n@@ -1 +1 @@\n-old\n+new" {
39+
t.Errorf("unexpected diff: %q", got)
40+
}
41+
}
42+
43+
func TestExtractDiffPlainBlockDashDash(t *testing.T) {
44+
// Plain block starting with --- header.
45+
response := "```\n--- a/main.go\n+++ b/main.go\n@@ -1 +1 @@\n-old\n+new\n```"
46+
got := extractDiff(response)
47+
if got == "" {
48+
t.Fatal("expected diff from --- block, got empty")
49+
}
50+
}
51+
52+
func TestExtractDiffPrefersTaggedBlock(t *testing.T) {
53+
// When both tagged and plain blocks exist, prefer tagged.
54+
response := "```\n--- a/wrong.go\n+++ b/wrong.go\n```\n\n```diff\n--- a/right.go\n+++ b/right.go\n```"
55+
got := extractDiff(response)
56+
if got != "--- a/right.go\n+++ b/right.go" {
57+
t.Errorf("expected tagged block to win, got %q", got)
58+
}
59+
}
60+
61+
func TestExtractDiffPlainBlockIgnoresNonDiff(t *testing.T) {
62+
// Plain code block that doesn't look like a diff should be ignored.
63+
response := "```\npackage main\nfunc main() {}\n```"
64+
got := extractDiff(response)
65+
if got != "" {
66+
t.Errorf("expected empty for non-diff plain block, got %q", got)
67+
}
68+
}

scripts/check-coverage.sh

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ EXCLUDED_FUNCTIONS=(
3838
"pkg/worker/ollama.go:.*applyDiff"
3939
"pkg/provider/codex.go:.*init"
4040
"pkg/provider/ollama.go:.*init"
41+
"pkg/linter/linter.go:.*normalizeIssuePaths"
4142
)
4243

4344
# Build grep exclusion pattern.

0 commit comments

Comments
 (0)