diff --git a/.github/workflows/acig.yml b/.github/workflows/acig.yml index ef9358c..8f90c5c 100644 --- a/.github/workflows/acig.yml +++ b/.github/workflows/acig.yml @@ -19,23 +19,31 @@ jobs: - name: Get GitHub App token id: app-token uses: actions/create-github-app-token@v2 - if: ${{ secrets.ACIG_APP_ID != '' && secrets.ACIG_APP_PRIVATE_KEY != '' }} + if: ${{ vars.ACIG_APP_ID != '' }} with: - app-id: ${{ secrets.ACIG_APP_ID }} + app-id: ${{ vars.ACIG_APP_ID }} private-key: ${{ secrets.ACIG_APP_PRIVATE_KEY }} - name: Install acig env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | - ACIG_VERSION="1.4.1" - RELEASE_URL="https://github.com/helloodokai/acig/releases/download/v${ACIG_VERSION}" + ACIG_VERSION=$(gh release view --repo helloodokai/acig --json tagName --jq '.tagName' 2>/dev/null || echo "") + if [ -z "$ACIG_VERSION" ]; then + echo "::error::could not determine latest acig version" + exit 1 + fi + echo "Using ACIG version ${ACIG_VERSION}" - curl -sL "${RELEASE_URL}/checksums.txt" -o /tmp/checksums.txt - curl -sL "${RELEASE_URL}/acig_linux_amd64.tar.gz" -o /tmp/acig_linux_amd64.tar.gz + gh release download "${ACIG_VERSION}" --repo helloodokai/acig -p "checksums.txt" -p "acig_linux_amd64.tar.gz" -D /tmp cd /tmp - EXPECTED=$(grep acig_linux_amd64.tar.gz checksums.txt | awk '{print $1}') + EXPECTED=$(grep "acig_linux_amd64.tar.gz" checksums.txt | awk '{print $1}') + if [ -z "$EXPECTED" ]; then + echo "::error::checksums.txt does not contain acig_linux_amd64.tar.gz" + cat checksums.txt + exit 1 + fi ACTUAL=$(sha256sum acig_linux_amd64.tar.gz | awk '{print $1}') if [ "$EXPECTED" != "$ACTUAL" ]; then echo "::error::checksum mismatch: expected $EXPECTED got $ACTUAL" diff --git a/.gitignore b/.gitignore index e5872a0..e9be4d4 100644 --- a/.gitignore +++ b/.gitignore @@ -28,4 +28,6 @@ Thumbs.db .acig.toml # Build -/acig \ No newline at end of file +/acig + +.charter.toml diff --git a/Makefile b/Makefile index 62f2e1e..fbdbb08 100644 --- a/Makefile +++ b/Makefile @@ -12,6 +12,8 @@ test: lint: golangci-lint run +check: lint test + vet: go vet ./... diff --git a/internal/critics/helpers.go b/internal/critics/helpers.go index 21b4b86..5c01600 100644 --- a/internal/critics/helpers.go +++ b/internal/critics/helpers.go @@ -36,6 +36,7 @@ type promptData struct { CriticalPaths string CriticResults string MaxFindings int + ChangedFiles string } func runCritic( @@ -221,11 +222,21 @@ func diffToPromptData(d *diff.Diff, cfg *Context) promptData { stats += fmt.Sprintf(" (showing first ~%d chars)", maxDiffChars) } + changedFiles := "" + if len(d.Files) > 0 { + var paths []string + for _, f := range d.Files { + paths = append(paths, f.Path) + } + changedFiles = strings.Join(paths, ", ") + } + return promptData{ Patch: patch, Stats: stats, CriticalPaths: criticalPaths, MaxFindings: 8, + ChangedFiles: changedFiles, } } diff --git a/internal/critics/prompts/adjudicator.md b/internal/critics/prompts/adjudicator.md index 175dbf7..0bccefd 100644 --- a/internal/critics/prompts/adjudicator.md +++ b/internal/critics/prompts/adjudicator.md @@ -7,6 +7,8 @@ Your job is to: 4. Add any critical findings the other critics may have missed. 5. Produce a final, authoritative set of findings. +CRITICAL: The "file" field in each finding MUST be an exact path from the original diff below. Do NOT invent or guess file paths that are not shown in the diff. Only use paths from this list: {{.ChangedFiles}}. Findings with fabricated file paths will be discarded. + IMPORTANT: Return at most {{.MaxFindings}} findings. Be concise. Use "blocking" only for findings that must prevent merge. Dismiss findings that are false positives. Previous critic results: diff --git a/internal/critics/prompts/perf_smell.md b/internal/critics/prompts/perf_smell.md index 7910337..6c2f35a 100644 --- a/internal/critics/prompts/perf_smell.md +++ b/internal/critics/prompts/perf_smell.md @@ -12,6 +12,8 @@ Focus on: - Inefficient string concatenation in loops - Missing context cancellation checks +CRITICAL: The "file" field in each finding MUST be an exact path from the diff below. Do NOT invent or guess file paths that are not shown in the diff. Only use paths from this list: {{.ChangedFiles}}. Findings with fabricated file paths will be discarded. + IMPORTANT: Return at most {{.MaxFindings}} findings. Be concise. Only report issues with measurable impact. Don't flag premature optimizations. If no issues, return: `{"findings": []}` diff --git a/internal/critics/prompts/risk_classifier.md b/internal/critics/prompts/risk_classifier.md index cd5d5a5..493b1c5 100644 --- a/internal/critics/prompts/risk_classifier.md +++ b/internal/critics/prompts/risk_classifier.md @@ -7,6 +7,8 @@ Consider: - Are there SQL injections, XSS, or other vulnerability patterns? - Are there unsafe operations (file I/O without checks, naked goroutines, etc.)? +CRITICAL: The "file" field in each finding MUST be an exact path from the diff below. Do NOT invent or guess file paths that are not shown in the diff. Only use paths from this list: {{.ChangedFiles}}. Findings with fabricated file paths will be discarded. + IMPORTANT: Return at most {{.MaxFindings}} findings. Be concise. Focus on the most significant issues only. Respond in JSON format: diff --git a/internal/critics/prompts/security_smell.md b/internal/critics/prompts/security_smell.md index 51973e3..accdc6b 100644 --- a/internal/critics/prompts/security_smell.md +++ b/internal/critics/prompts/security_smell.md @@ -12,6 +12,8 @@ Focus on: - Authorization issues (missing permission checks) - Sensitive data exposure in logs or responses +CRITICAL: The "file" field in each finding MUST be an exact path from the diff below. Do NOT invent or guess file paths that are not shown in the diff. Only use paths from this list: {{.ChangedFiles}}. Findings with fabricated file paths will be discarded. + IMPORTANT: Return at most {{.MaxFindings}} findings. Be concise. Only report GENUINE security concerns, not theoretical ones. Use "blocking" severity only for exploitable vulnerabilities. If no issues, return: `{"findings": []}` diff --git a/internal/critics/prompts/style_conformance.md b/internal/critics/prompts/style_conformance.md index e45dde9..db18f0e 100644 --- a/internal/critics/prompts/style_conformance.md +++ b/internal/critics/prompts/style_conformance.md @@ -9,6 +9,8 @@ Focus on: - Magic numbers or strings that should be constants - Dead code or unreachable paths +CRITICAL: The "file" field in each finding MUST be an exact path from the diff below. Do NOT invent or guess file paths that are not shown in the diff. Only use paths from this list: {{.ChangedFiles}}. Findings with fabricated file paths will be discarded. + IMPORTANT: Return at most {{.MaxFindings}} findings. Be concise. Only report actual issues. If the code looks fine, return: `{"findings": []}` diff --git a/internal/critics/prompts/test_coverage_smell.md b/internal/critics/prompts/test_coverage_smell.md index 48f5ba2..58b781e 100644 --- a/internal/critics/prompts/test_coverage_smell.md +++ b/internal/critics/prompts/test_coverage_smell.md @@ -7,6 +7,8 @@ Focus on: - New API endpoints without integration tests - Complex conditions needing boundary tests +CRITICAL: The "file" field in each finding MUST be an exact path from the diff below. Do NOT invent or guess file paths that are not shown in the diff. Only use paths from this list: {{.ChangedFiles}}. Findings with fabricated file paths will be discarded. + IMPORTANT: Return at most {{.MaxFindings}} findings. Be concise. Only report meaningful gaps. Trivial changes (constants, comments) don't need tests. If coverage is adequate, return: `{"findings": []}` diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 7fccf57..f37a50a 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -1,12 +1,19 @@ package diff +type HunkRange struct { + Start int + End int +} + type FileDiff struct { - Path string - Added []string - Removed []string - Patch string - IsNew bool - IsDelete bool + Path string + HunkRanges []HunkRange + StartLine int + Added []string + Removed []string + Patch string + IsNew bool + IsDelete bool } type Diff struct { diff --git a/internal/diff/diff_test.go b/internal/diff/diff_test.go index 86adc00..df67d68 100644 --- a/internal/diff/diff_test.go +++ b/internal/diff/diff_test.go @@ -38,8 +38,12 @@ func TestParse(t *testing.T) { require.Len(t, d.Files, 2) require.Equal(t, "main.go", d.Files[0].Path) require.True(t, d.Files[0].IsNew) + require.Equal(t, 1, d.Files[0].StartLine) + require.Equal(t, []HunkRange{{Start: 1, End: 5}}, d.Files[0].HunkRanges) require.Equal(t, "auth/login.go", d.Files[1].Path) require.False(t, d.Files[1].IsNew) + require.Equal(t, 10, d.Files[1].StartLine) + require.Equal(t, []HunkRange{{Start: 10, End: 17}}, d.Files[1].HunkRanges) require.Equal(t, 2, d.Stats.FilesChanged) } @@ -49,6 +53,47 @@ func TestParseEmpty(t *testing.T) { require.Len(t, d.Files, 0) } +func TestParseStartLine(t *testing.T) { + patch := `diff --git a/foo.go b/foo.go +index abc1234..def5678 100644 +--- a/foo.go ++++ b/foo.go +@@ -50,3 +50,5 @@ func existing() { ++ added line 1 ++ added line 2 + } +` + d, err := Parse(patch) + require.NoError(t, err) + require.Len(t, d.Files, 1) + require.Equal(t, "foo.go", d.Files[0].Path) + require.Equal(t, 50, d.Files[0].StartLine) + require.Equal(t, []HunkRange{{Start: 50, End: 54}}, d.Files[0].HunkRanges) + require.Equal(t, []string{"\tadded line 1", "\tadded line 2"}, d.Files[0].Added) +} + +func TestParseMultiHunkRanges(t *testing.T) { + patch := `diff --git a/bar.go b/bar.go +index 1111111..2222222 100644 +--- a/bar.go ++++ b/bar.go +@@ -10,3 +10,5 @@ func first() { + context1 ++ added1 + context2 +@@ -50,3 +50,4 @@ func second() { + context3 ++ added2 + context4 +} +` + d, err := Parse(patch) + require.NoError(t, err) + require.Len(t, d.Files, 1) + require.Equal(t, 10, d.Files[0].StartLine) + require.Equal(t, []HunkRange{{Start: 10, End: 14}, {Start: 50, End: 53}}, d.Files[0].HunkRanges) +} + func TestNewPath(t *testing.T) { tests := []struct { name string diff --git a/internal/diff/git.go b/internal/diff/git.go index 5de9166..c7eecc8 100644 --- a/internal/diff/git.go +++ b/internal/diff/git.go @@ -83,6 +83,15 @@ func Parse(patch string) (*Diff, error) { } for _, h := range f.Hunks { + if h.NewStartLine > 0 && h.NewLines > 0 { + fd.HunkRanges = append(fd.HunkRanges, HunkRange{ + Start: int(h.NewStartLine), + End: int(h.NewStartLine) + int(h.NewLines) - 1, + }) + } + if fd.StartLine == 0 && h.NewStartLine > 0 { + fd.StartLine = int(h.NewStartLine) + } lines := bytes.Split(h.Body, []byte{'\n'}) for _, l := range lines { line := string(l) diff --git a/internal/githubclient/review.go b/internal/githubclient/review.go index c872a53..3defb00 100644 --- a/internal/githubclient/review.go +++ b/internal/githubclient/review.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/google/go-github/v66/github" + "github.com/helloodokai/acig/internal/diff" ) func (c *Client) ListReviews(ctx context.Context, owner, repo string, prNumber int) ([]*github.PullRequestReview, error) { @@ -99,4 +100,40 @@ func (c *Client) RemoveStaleAcigComments(ctx context.Context, owner, repo string } } } +} + +func (c *Client) ListPRFiles(ctx context.Context, owner, repo string, prNumber int) ([]string, error) { + files, _, err := c.client.PullRequests.ListFiles(ctx, owner, repo, prNumber, &github.ListOptions{PerPage: 100}) + if err != nil { + return nil, fmt.Errorf("listing PR files: %w", err) + } + paths := make([]string, len(files)) + for i, f := range files { + paths[i] = f.GetFilename() + } + return paths, nil +} + +func (c *Client) GetPRFileDiffs(ctx context.Context, owner, repo string, prNumber int) (map[string]*diff.FileDiff, error) { + files, _, err := c.client.PullRequests.ListFiles(ctx, owner, repo, prNumber, &github.ListOptions{PerPage: 100}) + if err != nil { + return nil, fmt.Errorf("listing PR files for diffs: %w", err) + } + + result := make(map[string]*diff.FileDiff) + for _, f := range files { + if f.Patch == nil || *f.Patch == "" { + continue + } + d, parseErr := diff.FromFiles(*f.Patch) + if parseErr != nil || len(d.Files) == 0 { + continue + } + fd := &d.Files[0] + if fd.IsDelete { + continue + } + result[fd.Path] = fd + } + return result, nil } \ No newline at end of file diff --git a/internal/githubclient/review_test.go b/internal/githubclient/review_test.go new file mode 100644 index 0000000..1a1fd70 --- /dev/null +++ b/internal/githubclient/review_test.go @@ -0,0 +1,455 @@ +package githubclient + +import ( + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "github.com/google/go-github/v66/github" + "github.com/helloodokai/acig/internal/diff" + "github.com/stretchr/testify/require" +) + +func newTestClient(server *httptest.Server) *Client { + httpClient := server.Client() + ghClient := github.NewClient(httpClient).WithAuthToken("fake-token") + ghClient.BaseURL, _ = url.Parse(server.URL + "/") + return &Client{client: ghClient} +} + +func TestGetPRFileDiffs(t *testing.T) { + patchAuthLogin := `diff --git a/auth/login.go b/auth/login.go +index 1234567..abcdef0 100644 +--- a/auth/login.go ++++ b/auth/login.go +@@ -10,6 +10,8 @@ func Login(username string) error { + if err != nil { +- return err +- } ++ if err != nil { ++ log.Printf("login failed: %v", err) ++ return fmt.Errorf("login: %w", err) ++ } +` + + patchMultiHunk := `diff --git a/bar.go b/bar.go +index 1111111..2222222 100644 +--- a/bar.go ++++ b/bar.go +@@ -10,3 +10,5 @@ func first() { + context1 ++ added1 + context2 +@@ -50,3 +50,4 @@ func second() { + context3 ++ added2 + context4 +} +` + + patchDeletedFile := `diff --git a/old_file.go b/old_file.go +deleted file mode 100644 +index abc1234..0000000 +--- a/old_file.go ++++ /dev/null +@@ -1,5 +0,0 @@ +-package old +- +-func removed() { +-} +` + + type ghFile struct { + Filename string `json:"filename"` + Patch string `json:"patch,omitempty"` + Status string `json:"status,omitempty"` + Additions int `json:"additions,omitempty"` + Deletions int `json:"deletions,omitempty"` + Changes int `json:"changes,omitempty"` + } + + tests := []struct { + name string + files []ghFile + expectLen int + expectPaths []string + expectHunks map[string][]diff.HunkRange + expectNew map[string]bool + expectStart map[string]int + }{ + { + name: "single file with patch", + files: []ghFile{ + { + Filename: "auth/login.go", + Patch: patchAuthLogin, + Status: "modified", + Additions: 4, + Deletions: 2, + Changes: 6, + }, + }, + expectLen: 1, + expectPaths: []string{"auth/login.go"}, + expectHunks: map[string][]diff.HunkRange{ + "auth/login.go": {{Start: 10, End: 17}}, + }, + expectNew: map[string]bool{ + "auth/login.go": false, + }, + expectStart: map[string]int{ + "auth/login.go": 10, + }, + }, + { + name: "multi-hunk file", + files: []ghFile{ + { + Filename: "bar.go", + Patch: patchMultiHunk, + Status: "modified", + Additions: 2, + Deletions: 0, + Changes: 2, + }, + }, + expectLen: 1, + expectPaths: []string{"bar.go"}, + expectHunks: map[string][]diff.HunkRange{ + "bar.go": {{Start: 10, End: 14}, {Start: 50, End: 53}}, + }, + expectStart: map[string]int{ + "bar.go": 10, + }, + }, + { + name: "filters out deleted files", + files: []ghFile{ + { + Filename: "auth/login.go", + Patch: patchAuthLogin, + Status: "modified", + Additions: 4, + Deletions: 2, + Changes: 6, + }, + { + Filename: "old_file.go", + Patch: patchDeletedFile, + Status: "removed", + Additions: 0, + Deletions: 5, + Changes: 5, + }, + }, + expectLen: 1, + expectPaths: []string{"auth/login.go"}, + expectHunks: map[string][]diff.HunkRange{ + "auth/login.go": {{Start: 10, End: 17}}, + }, + }, + { + name: "filters out files without patch", + files: []ghFile{ + { + Filename: "binary_file.png", + Status: "modified", + }, + { + Filename: "auth/login.go", + Patch: patchAuthLogin, + Status: "modified", + Additions: 4, + Deletions: 2, + Changes: 6, + }, + }, + expectLen: 1, + expectPaths: []string{"auth/login.go"}, + }, + { + name: "filters out file with empty patch", + files: []ghFile{ + { + Filename: "empty_patch.go", + Patch: "", + Status: "modified", + }, + { + Filename: "auth/login.go", + Patch: patchAuthLogin, + Status: "modified", + Additions: 4, + Deletions: 2, + Changes: 6, + }, + }, + expectLen: 1, + expectPaths: []string{"auth/login.go"}, + }, + { + name: "empty response", + files: []ghFile{}, + expectLen: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/repos/owner/repo/pulls/42/files" { + body, _ := json.Marshal(tt.files) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write(body) + return + } + http.NotFound(w, r) + })) + defer server.Close() + + client := newTestClient(server) + + result, err := client.GetPRFileDiffs(context.Background(), "owner", "repo", 42) + require.NoError(t, err) + require.Len(t, result, tt.expectLen) + + for _, path := range tt.expectPaths { + fd, ok := result[path] + require.True(t, ok, "expected file %q in result", path) + if hunks, expected := tt.expectHunks[path]; expected { + require.Equal(t, hunks, fd.HunkRanges, "hunk ranges for %q", path) + } + if isNew, expected := tt.expectNew[path]; expected { + require.Equal(t, isNew, fd.IsNew, "IsNew for %q", path) + } + if startLine, expected := tt.expectStart[path]; expected { + require.Equal(t, startLine, fd.StartLine, "StartLine for %q", path) + } + } + }) + } +} + +func TestGetPRFileDiffs_PathNormalization(t *testing.T) { + patchNewFile := `diff --git a/main.go b/main.go +new file mode 100644 +index 0000000..abc1234 +--- /dev/null ++++ b/main.go +@@ -0,0 +1,5 @@ ++package main ++ ++func main() { ++ println("hello") ++} +` + + files := []struct { + Filename string `json:"filename"` + Patch string `json:"patch"` + Status string `json:"status"` + Additions int `json:"additions"` + Deletions int `json:"deletions"` + Changes int `json:"changes"` + }{ + { + Filename: "main.go", + Patch: patchNewFile, + Status: "added", + Additions: 5, + Changes: 5, + }, + } + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/repos/owner/repo/pulls/1/files" { + body, _ := json.Marshal(files) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write(body) + return + } + http.NotFound(w, r) + })) + defer server.Close() + + client := newTestClient(server) + + result, err := client.GetPRFileDiffs(context.Background(), "owner", "repo", 1) + require.NoError(t, err) + require.Len(t, result, 1) + + fd, ok := result["main.go"] + require.True(t, ok, "expected 'main.go' in result, got keys: %v", mapKeys(result)) + require.True(t, fd.IsNew, "expected IsNew=true for new file") + require.Equal(t, 1, fd.StartLine) + require.Equal(t, []diff.HunkRange{{Start: 1, End: 5}}, fd.HunkRanges) +} + +func TestGetPRFileDiffs_APIError(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"message":"Internal Server Error"}`)) + })) + defer server.Close() + + client := newTestClient(server) + + _, err := client.GetPRFileDiffs(context.Background(), "owner", "repo", 1) + require.Error(t, err) +} + +func TestCreateReview(t *testing.T) { + type createReviewRequest struct { + Body string `json:"body"` + Event string `json:"event"` + Comments []struct { + Path string `json:"path"` + Line int `json:"line"` + StartLine int `json:"start_line,omitempty"` + Side string `json:"side"` + StartSide string `json:"start_side,omitempty"` + Body string `json:"body"` + } `json:"comments"` + } + + tests := []struct { + name string + comments []ReviewComment + event string + body string + want createReviewRequest + }{ + { + name: "single-line comment", + event: "COMMENT", + body: "LGTM", + comments: []ReviewComment{ + {Path: "main.go", Line: 10, StartLine: 0, Body: "nit: fix this"}, + }, + want: createReviewRequest{ + Body: "LGTM", + Event: "COMMENT", + Comments: []struct { + Path string `json:"path"` + Line int `json:"line"` + StartLine int `json:"start_line,omitempty"` + Side string `json:"side"` + StartSide string `json:"start_side,omitempty"` + Body string `json:"body"` + }{ + {Path: "main.go", Line: 10, Side: "RIGHT", Body: "nit: fix this"}, + }, + }, + }, + { + name: "multi-line comment", + event: "REQUEST_CHANGES", + body: "Please fix", + comments: []ReviewComment{ + {Path: "auth/login.go", Line: 25, StartLine: 20, Body: "refactor this block"}, + }, + want: createReviewRequest{ + Body: "Please fix", + Event: "REQUEST_CHANGES", + Comments: []struct { + Path string `json:"path"` + Line int `json:"line"` + StartLine int `json:"start_line,omitempty"` + Side string `json:"side"` + StartSide string `json:"start_side,omitempty"` + Body string `json:"body"` + }{ + {Path: "auth/login.go", Line: 25, StartLine: 20, Side: "RIGHT", StartSide: "RIGHT", Body: "refactor this block"}, + }, + }, + }, + { + name: "skips comments with line <= 0", + event: "COMMENT", + body: "review", + comments: []ReviewComment{ + {Path: "main.go", Line: 0, Body: "skip me"}, + {Path: "other.go", Line: 5, Body: "keep me"}, + }, + want: createReviewRequest{ + Body: "review", + Event: "COMMENT", + Comments: []struct { + Path string `json:"path"` + Line int `json:"line"` + StartLine int `json:"start_line,omitempty"` + Side string `json:"side"` + StartSide string `json:"start_side,omitempty"` + Body string `json:"body"` + }{ + {Path: "other.go", Line: 5, Side: "RIGHT", Body: "keep me"}, + }, + }, + }, + { + name: "no comments", + event: "APPROVE", + body: "Looks good", + comments: nil, + want: createReviewRequest{ + Body: "Looks good", + Event: "APPROVE", + Comments: nil, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var received createReviewRequest + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost && r.URL.Path == "/repos/owner/repo/pulls/42/reviews" { + body, _ := io.ReadAll(r.Body) + defer r.Body.Close() + _ = json.Unmarshal(body, &received) + + resp := `{"id":1,"user":{"login":"test"},"body":"","state":"APPROVED"}` + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(resp)) + return + } + http.NotFound(w, r) + })) + defer server.Close() + + client := newTestClient(server) + + err := client.CreateReview(context.Background(), "owner", "repo", 42, tt.body, tt.comments, tt.event) + require.NoError(t, err) + + require.Equal(t, tt.want.Body, received.Body) + require.Equal(t, tt.want.Event, received.Event) + require.Len(t, received.Comments, len(tt.want.Comments)) + + for i, want := range tt.want.Comments { + got := received.Comments[i] + require.Equal(t, want.Path, got.Path, "comment %d: path", i) + require.Equal(t, want.Line, got.Line, "comment %d: line", i) + require.Equal(t, want.Side, got.Side, "comment %d: side", i) + require.Equal(t, want.Body, got.Body, "comment %d: body", i) + if want.StartLine > 0 { + require.Equal(t, want.StartLine, got.StartLine, "comment %d: start_line", i) + require.Equal(t, want.StartSide, got.StartSide, "comment %d: start_side", i) + } + } + }) + } +} + +func mapKeys(m map[string]*diff.FileDiff) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + return keys +} \ No newline at end of file diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 7588f89..a79f53b 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -163,7 +163,12 @@ func (p *Pipeline) Execute(ctx context.Context, repo, sha, baseSHA string) (*ver } } - finalize(pc.Result, p.ledger, p.suppressions) + diffPaths := make([]string, len(p.d.Files)) + for i, f := range p.d.Files { + diffPaths[i] = f.Path + } + + finalize(pc.Result, p.ledger, p.suppressions, diffPaths) return pc.Result, nil } @@ -198,9 +203,18 @@ func hasConflict(results []verdict.CriticResult) bool { return len(severityCounts) >= 3 } -func finalize(v *verdict.Verdict, ledger *budget.Ledger, suppressions []verdict.Suppression) { +func finalize(v *verdict.Verdict, ledger *budget.Ledger, suppressions []verdict.Suppression, diffPaths []string) { v.Findings = verdict.DedupeFindings(v.Findings) - v.Findings = verdict.FilterFindings(v.Findings, suppressions) + + inDiff, dangling, hallucinated := verdict.CategorizeFindingsByPath(v.Findings, diffPaths) + dropped := len(hallucinated) + + if dropped > 0 { + slog.Warn("filtered hallucinated file paths", "count", dropped, "dangling_count", len(dangling)) + } + + v.DanglingFindings = verdict.FilterFindings(dangling, suppressions) + v.Findings = verdict.FilterFindings(inDiff, suppressions) v.TotalCostUSD = ledger.Spent() v.BudgetRemainingUSD = ledger.Remaining() v.Risk = computeRisk(v.Findings, v.Risk) diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 8950800..3c6a4d2 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -3,7 +3,9 @@ package pipeline import ( "testing" + "github.com/helloodokai/acig/internal/budget" "github.com/helloodokai/acig/internal/verdict" + "github.com/stretchr/testify/require" ) func TestComputeDecision(t *testing.T) { @@ -83,6 +85,56 @@ func TestComputeRisk(t *testing.T) { } } +func TestFinalize_CategorizePaths(t *testing.T) { + v := &verdict.Verdict{ + Findings: []verdict.Finding{ + {File: "apps/backend/src/lib/tools/plan-tools.ts", LineStart: 10, Severity: verdict.SeverityMedium, Title: "real issue"}, + {File: "apps/backend/src/lib/tools/plan-tools.test.ts", LineStart: 0, Severity: verdict.SeverityLow, Critic: "test_coverage_smell", Title: "missing tests"}, + {File: "service/get_plan_context.go", LineStart: 5, Severity: verdict.SeverityHigh, Critic: "perf_smell", Title: "hallucinated Go file"}, + }, + } + ledger, _ := budget.NewLedger(1.0) + diffPaths := []string{"apps/backend/src/lib/tools/plan-tools.ts", ".charters/ch-2026-05-07-12f7a9.spec.md"} + + finalize(v, ledger, nil, diffPaths) + + require.Len(t, v.Findings, 1) + require.Equal(t, "apps/backend/src/lib/tools/plan-tools.ts", v.Findings[0].File) + require.Len(t, v.DanglingFindings, 1) + require.Equal(t, "apps/backend/src/lib/tools/plan-tools.test.ts", v.DanglingFindings[0].File) +} + +func TestFinalize_EmptyDiffPathsPreservesAll(t *testing.T) { + v := &verdict.Verdict{ + Findings: []verdict.Finding{ + {File: "any/file.go", LineStart: 1, Severity: verdict.SeverityLow, Title: "some issue"}, + }, + } + ledger, _ := budget.NewLedger(1.0) + + finalize(v, ledger, nil, nil) + + require.Len(t, v.Findings, 1) +} + +func TestFinalize_SuppressionsAppliedAfterPathFilter(t *testing.T) { + suppressions := []verdict.Suppression{ + {Critic: "perf_smell", Title: "real issue", Reason: "known false positive"}, + } + v := &verdict.Verdict{ + Findings: []verdict.Finding{ + {File: "real.ts", LineStart: 10, Severity: verdict.SeverityHigh, Critic: "perf_smell", Title: "real issue"}, + {File: "hallucinated.go", LineStart: 5, Severity: verdict.SeverityMedium, Critic: "perf_smell", Title: "hallucinated"}, + }, + } + ledger, _ := budget.NewLedger(1.0) + diffPaths := []string{"real.ts"} + + finalize(v, ledger, suppressions, diffPaths) + + require.Len(t, v.Findings, 0) +} + func TestClassifyRisk(t *testing.T) { tests := []struct { name string diff --git a/internal/reporters/github.go b/internal/reporters/github.go index d075248..8efc876 100644 --- a/internal/reporters/github.go +++ b/internal/reporters/github.go @@ -7,6 +7,8 @@ import ( "log/slog" "strings" + "github.com/google/go-github/v66/github" + "github.com/helloodokai/acig/internal/diff" "github.com/helloodokai/acig/internal/githubclient" "github.com/helloodokai/acig/internal/verdict" ) @@ -14,11 +16,23 @@ import ( const acigMarker = "" type GitHubReporter struct { - client *githubclient.Client + client GitHubClient headSHA string } -func NewGitHubReporter(client *githubclient.Client, headSHA string) *GitHubReporter { +type GitHubClient interface { + ListPRFiles(ctx context.Context, owner, repo string, prNumber int) ([]string, error) + GetPRFileDiffs(ctx context.Context, owner, repo string, prNumber int) (map[string]*diff.FileDiff, error) + ListReviews(ctx context.Context, owner, repo string, prNumber int) ([]*github.PullRequestReview, error) + DeleteReviewComments(ctx context.Context, owner, repo string, prNumber int, reviewID int64) error + DismissReview(ctx context.Context, owner, repo string, prNumber int, reviewID int64, message string) error + CreateReview(ctx context.Context, owner, repo string, prNumber int, body string, comments []githubclient.ReviewComment, event string) error + PostStickyComment(ctx context.Context, owner, repo string, prNumber int, marker, body string) error + RemoveStaleAcigComments(ctx context.Context, owner, repo string, prNumber int, marker string) + CreateCheckRun(ctx context.Context, owner, repo, name, conclusion, title, summary, headSHA string) error +} + +func NewGitHubReporter(client GitHubClient, headSHA string) *GitHubReporter { return &GitHubReporter{client: client, headSHA: headSHA} } @@ -30,25 +44,41 @@ func (r *GitHubReporter) Report(ctx context.Context, v *verdict.Verdict, owner, r.client.RemoveStaleAcigComments(ctx, owner, repo, prNumber, acigMarker) reviewBody := r.buildReviewBody(v) - reviewComments := r.buildReviewComments(v) + + prFiles, err := r.client.ListPRFiles(ctx, owner, repo, prNumber) + if err != nil { + slog.Warn("failed to list PR files, using all findings", "error", err) + prFiles = nil + } + + fileDiffs, err := r.client.GetPRFileDiffs(ctx, owner, repo, prNumber) + if err != nil { + slog.Warn("failed to get PR file diffs, skipping line validation", "error", err) + fileDiffs = nil + } + + reviewComments := buildReviewComments(v, prFiles, fileDiffs) event := "COMMENT" if v.Decision == verdict.DecisionBlock { event = "REQUEST_CHANGES" } if err := r.client.CreateReview(ctx, owner, repo, prNumber, reviewBody, reviewComments, event); err != nil { - slog.Warn("failed to create review, falling back to comment", "error", err) - md := FormatMarkdown(v) - var body strings.Builder - body.WriteString(acigMarker + "\n") - body.WriteString(md) - body.WriteString("\n\n
\nVerdict JSON\n\n```json\n") - jsonBody, err := verdictJSON(v) - if err == nil { - body.WriteString(jsonBody) + slog.Warn("failed to create review with comments, retrying without inline comments", "error", err) + if err := r.client.CreateReview(ctx, owner, repo, prNumber, reviewBody, nil, event); err != nil { + slog.Warn("failed to create review, falling back to comment", "error", err) + md := FormatMarkdown(v) + var body strings.Builder + body.WriteString(acigMarker + "\n") + body.WriteString(md) + body.WriteString("\n\n
\nVerdict JSON\n\n```json\n") + jsonBody, err := verdictJSON(v) + if err == nil { + body.WriteString(jsonBody) + } + body.WriteString("\n```\n
\n") + return r.client.PostStickyComment(ctx, owner, repo, prNumber, acigMarker, body.String()) } - body.WriteString("\n```\n
\n") - return r.client.PostStickyComment(ctx, owner, repo, prNumber, acigMarker, body.String()) } conclusion := "success" @@ -99,18 +129,61 @@ func (r *GitHubReporter) dismissOldReviews(ctx context.Context, owner, repo stri func (r *GitHubReporter) buildReviewBody(v *verdict.Verdict) string { var body strings.Builder body.WriteString(acigMarker + "\n") - body.WriteString(fmt.Sprintf("## acig: %s | risk=%s | %d finding(s) | $%.4f\n\n", v.Decision, v.Risk, len(v.Findings), v.TotalCostUSD)) - body.WriteString("| Severity | Critic | Title | File |\n|----------|--------|-------|------|\n") - for _, f := range v.Findings { - file := f.File - if file == "" { - file = "—" + decisionEmoji := "✅" + switch v.Decision { + case verdict.DecisionWarn: + decisionEmoji = "⚠ī¸" + case verdict.DecisionBlock: + decisionEmoji = "đŸšĢ" + } + body.WriteString(fmt.Sprintf("## %s acig verdict: %s | risk=%s | %d finding(s) | $%.4f\n\n", + decisionEmoji, strings.ToUpper(string(v.Decision)), v.Risk, len(v.Findings)+len(v.DanglingFindings), v.TotalCostUSD)) + + if len(v.Findings) > 0 { + body.WriteString("### Inline Findings\n\n") + body.WriteString("| Severity | Critic | Title | File |\n|----------|--------|-------|------|\n") + for _, f := range v.Findings { + file := f.File + if file == "" { + file = "—" + } + body.WriteString(fmt.Sprintf("| %s | %s | %s | %s |\n", severityEmoji(f.Severity), f.Critic, f.Title, file)) + } + body.WriteString("\n") + } + + if len(v.DanglingFindings) > 0 { + body.WriteString("### Observations (files not in this PR)\n\n") + body.WriteString("The following findings reference files not changed in this PR. They are legitimate suggestions but cannot be placed as inline review comments.\n\n") + for _, f := range v.DanglingFindings { + body.WriteString(fmt.Sprintf("- %s **%s** (%s)%s\n", + severityEmoji(f.Severity), f.Title, f.Critic, formatFileNote(f))) + if f.Detail != "" { + body.WriteString(fmt.Sprintf(" \n %s\n", f.Detail)) + } + if f.SuggestedFix != "" { + body.WriteString(fmt.Sprintf(" \n **Suggested fix:** %s\n", f.SuggestedFix)) + } } - body.WriteString(fmt.Sprintf("| %s | %s | %s | %s |\n", f.Severity, f.Critic, f.Title, file)) + body.WriteString("\n") } - body.WriteString("\n
\nVerdict JSON\n\n```json\n") + if len(v.CriticResults) > 0 { + body.WriteString("
\nCritic Results\n\n") + body.WriteString("| Critic | Model | Findings | Cost | Duration |\n|--------|-------|----------|------|----------|\n") + for _, cr := range v.CriticResults { + errIndicator := "" + if cr.Error != "" { + errIndicator = " ⚠ī¸" + } + body.WriteString(fmt.Sprintf("| %s%s | %s | %d | $%.4f | %dms |\n", + cr.Critic, errIndicator, cr.Model, len(cr.Findings), cr.CostUSD, cr.DurationMS)) + } + body.WriteString("\n
\n\n") + } + + body.WriteString("
\nVerdict JSON\n\n```json\n") jsonBody, err := verdictJSON(v) if err == nil { body.WriteString(jsonBody) @@ -120,14 +193,76 @@ func (r *GitHubReporter) buildReviewBody(v *verdict.Verdict) string { return body.String() } -func (r *GitHubReporter) buildReviewComments(v *verdict.Verdict) []githubclient.ReviewComment { +func severityEmoji(s verdict.Severity) string { + switch s { + case verdict.SeverityBlocking: + return "đŸšĢ" + case verdict.SeverityHigh: + return "🔴" + case verdict.SeverityMedium: + return "🟡" + case verdict.SeverityLow: + return "đŸŸĸ" + case verdict.SeverityInfo: + return "ℹī¸" + default: + return string(s) + } +} + +func formatFileNote(f verdict.Finding) string { + if f.File == "" { + return "" + } + return fmt.Sprintf(" — `%s`", f.File) +} + +func buildReviewComments(v *verdict.Verdict, prFiles []string, fileDiffs map[string]*diff.FileDiff) []githubclient.ReviewComment { fileFindings := groupFindings(v.Findings) var comments []githubclient.ReviewComment + prFilesSet := make(map[string]bool, len(prFiles)) + for _, f := range prFiles { + prFilesSet[f] = true + } + for _, findings := range fileFindings { - if len(findings) == 0 || findings[0].File == "" || findings[0].LineStart <= 0 { + if len(findings) == 0 || findings[0].File == "" { + continue + } + if len(prFilesSet) > 0 && !prFilesSet[findings[0].File] { + slog.Warn("skipping finding for file not in PR", "file", findings[0].File, "critic", findings[0].Critic, "title", findings[0].Title) + continue + } + + fd := fileDiffs[findings[0].File] + if fd == nil { + slog.Warn("skipping finding with no diff data", "file", findings[0].File, "critic", findings[0].Critic, "title", findings[0].Title) continue } + + lineStart := findings[0].LineStart + if lineStart <= 0 { + if len(fd.HunkRanges) == 0 { + continue + } + lineStart = fd.HunkRanges[0].Start + } + + validStart := validateLine(fd, lineStart) + if validStart <= 0 { + continue + } + + commentLineStart := validStart + commentLineEnd := commentLineStart + if findings[0].LineEnd > lineStart { + validEnd := validateLine(fd, findings[0].LineEnd) + if validEnd > 0 { + commentLineEnd = validEnd + } + } + var body strings.Builder body.WriteString(fmt.Sprintf("**acig** found %d issue(s) here:\n\n", len(findings))) for i, f := range findings { @@ -141,12 +276,12 @@ func (r *GitHubReporter) buildReviewComments(v *verdict.Verdict) []githubclient. } comment := githubclient.ReviewComment{ Path: findings[0].File, - Line: findings[0].LineStart, + Line: commentLineStart, Body: body.String(), } - if findings[0].LineEnd > findings[0].LineStart { - comment.Line = findings[0].LineEnd - comment.StartLine = findings[0].LineStart + if commentLineEnd > commentLineStart { + comment.Line = commentLineEnd + comment.StartLine = commentLineStart } comments = append(comments, comment) } @@ -158,10 +293,14 @@ func groupFindings(findings []verdict.Finding) [][]verdict.Finding { groups := map[string][]verdict.Finding{} var order []string for _, f := range findings { - if f.File == "" || f.LineStart <= 0 { + if f.File == "" { continue } - key := fmt.Sprintf("%s:%d", f.File, f.LineStart) + line := f.LineStart + if line <= 0 { + line = 0 + } + key := fmt.Sprintf("%s:%d", f.File, line) if _, exists := groups[key]; !exists { order = append(order, key) } @@ -175,6 +314,34 @@ func groupFindings(findings []verdict.Finding) [][]verdict.Finding { return result } +func validateLine(fd *diff.FileDiff, requestedLine int) int { + if fd == nil || fd.IsDelete { + return 0 + } + if len(fd.HunkRanges) == 0 { + return 0 + } + for _, hr := range fd.HunkRanges { + if requestedLine >= hr.Start && requestedLine <= hr.End { + return requestedLine + } + } + nearest := fd.HunkRanges[0] + for _, hr := range fd.HunkRanges[1:] { + if abs(requestedLine-hr.Start) < abs(requestedLine-nearest.Start) { + nearest = hr + } + } + return nearest.Start +} + +func abs(x int) int { + if x < 0 { + return -x + } + return x +} + func verdictJSON(v *verdict.Verdict) (string, error) { b, err := json.Marshal(v) if err != nil { diff --git a/internal/reporters/github_report_test.go b/internal/reporters/github_report_test.go new file mode 100644 index 0000000..05abb5c --- /dev/null +++ b/internal/reporters/github_report_test.go @@ -0,0 +1,170 @@ +package reporters + +import ( + "context" + "errors" + "testing" + + "github.com/google/go-github/v66/github" + "github.com/helloodokai/acig/internal/diff" + "github.com/helloodokai/acig/internal/githubclient" + "github.com/helloodokai/acig/internal/verdict" + "github.com/stretchr/testify/require" +) + +type mockGitHubClient struct { + createReviewCalls []createReviewCall + createReviewFailAt int + postStickyCommentErr error + listReviewsErr error + deleteReviewCommentsErr error + dismissReviewErr error + getPRFileDiffsErr error +} + +type createReviewCall struct { + comments []githubclient.ReviewComment +} + +func (m *mockGitHubClient) ListReviews(ctx context.Context, owner, repo string, prNumber int) ([]*github.PullRequestReview, error) { + if m.listReviewsErr != nil { + return nil, m.listReviewsErr + } + return nil, nil +} + +func (m *mockGitHubClient) DeleteReviewComments(ctx context.Context, owner, repo string, prNumber int, reviewID int64) error { + return m.deleteReviewCommentsErr +} + +func (m *mockGitHubClient) DismissReview(ctx context.Context, owner, repo string, prNumber int, reviewID int64, message string) error { + return m.dismissReviewErr +} + +func (m *mockGitHubClient) ListPRFiles(ctx context.Context, owner, repo string, prNumber int) ([]string, error) { + return []string{"a.txt"}, nil +} + +func (m *mockGitHubClient) GetPRFileDiffs(ctx context.Context, owner, repo string, prNumber int) (map[string]*diff.FileDiff, error) { + if m.getPRFileDiffsErr != nil { + return nil, m.getPRFileDiffsErr + } + return map[string]*diff.FileDiff{ + "a.txt": {Path: "a.txt", StartLine: 1, HunkRanges: []diff.HunkRange{{Start: 1, End: 15}}, Added: []string{"line1", "line2", "line3", "line4", "line5", "line6", "line7", "line8", "line9", "line10"}}, + }, nil +} + +func (m *mockGitHubClient) CreateReview(ctx context.Context, owner, repo string, prNumber int, body string, comments []githubclient.ReviewComment, event string) error { + m.createReviewCalls = append(m.createReviewCalls, createReviewCall{comments: comments}) + if m.createReviewFailAt > 0 && len(m.createReviewCalls) >= m.createReviewFailAt { + return errors.New("position could not be resolved") + } + return nil +} + +func (m *mockGitHubClient) PostStickyComment(ctx context.Context, owner, repo string, prNumber int, marker, body string) error { + return m.postStickyCommentErr +} + +func (m *mockGitHubClient) RemoveStaleAcigComments(ctx context.Context, owner, repo string, prNumber int, marker string) {} + +func (m *mockGitHubClient) CreateCheckRun(ctx context.Context, owner, repo, name, conclusion, title, summary, headSHA string) error { + return nil +} + +func TestReport_RetryWithoutCommentsOnPositionError(t *testing.T) { + mock := &mockGitHubClient{ + createReviewFailAt: 1, + } + + reporter := &GitHubReporter{ + client: mock, + } + + v := &verdict.Verdict{ + Decision: verdict.DecisionPass, + Risk: verdict.RiskLow, + Findings: []verdict.Finding{ + {File: "a.txt", LineStart: 10, Title: "Issue A"}, + }, + } + + err := reporter.Report(context.Background(), v, "owner", "repo", 1) + require.NoError(t, err) + + require.Len(t, mock.createReviewCalls, 2) + require.NotNil(t, mock.createReviewCalls[0].comments) + require.Nil(t, mock.createReviewCalls[1].comments) +} + +func TestReport_FallsBackToStickyCommentWhenBothReviewAttemptsFail(t *testing.T) { + mock := &mockGitHubClient{ + createReviewFailAt: 1, + postStickyCommentErr: errors.New("sticky comment failed"), + } + + reporter := &GitHubReporter{ + client: mock, + } + + v := &verdict.Verdict{ + Decision: verdict.DecisionPass, + Risk: verdict.RiskLow, + Findings: []verdict.Finding{ + {File: "a.txt", LineStart: 10, Title: "Issue A"}, + }, + } + + err := reporter.Report(context.Background(), v, "owner", "repo", 1) + require.Error(t, err) + require.Len(t, mock.createReviewCalls, 2) + require.Contains(t, mock.postStickyCommentErr.Error(), "sticky comment failed") +} + +func TestReport_SuccessfulReviewOnFirstTry(t *testing.T) { + mock := &mockGitHubClient{ + createReviewFailAt: 0, + } + + reporter := &GitHubReporter{ + client: mock, + } + + v := &verdict.Verdict{ + Decision: verdict.DecisionPass, + Risk: verdict.RiskLow, + Findings: []verdict.Finding{ + {File: "a.txt", LineStart: 10, Title: "Issue A"}, + }, + } + + err := reporter.Report(context.Background(), v, "owner", "repo", 1) + require.NoError(t, err) + + require.Len(t, mock.createReviewCalls, 1) + require.NotNil(t, mock.createReviewCalls[0].comments) +} + +func TestReport_UsesPRFilesFilter(t *testing.T) { + mock := &mockGitHubClient{} + + reporter := &GitHubReporter{ + client: mock, + } + + v := &verdict.Verdict{ + Decision: verdict.DecisionPass, + Risk: verdict.RiskLow, + Findings: []verdict.Finding{ + {File: "a.txt", LineStart: 10, Title: "Issue A"}, + {File: "deleted.txt", LineStart: 20, Title: "Should be filtered"}, + }, + } + + err := reporter.Report(context.Background(), v, "owner", "repo", 1) + require.NoError(t, err) + + require.Len(t, mock.createReviewCalls, 1) + require.Len(t, mock.createReviewCalls[0].comments, 1) + require.Equal(t, "a.txt", mock.createReviewCalls[0].comments[0].Path) +} \ No newline at end of file diff --git a/internal/reporters/github_test.go b/internal/reporters/github_test.go new file mode 100644 index 0000000..35239f7 --- /dev/null +++ b/internal/reporters/github_test.go @@ -0,0 +1,355 @@ +package reporters + +import ( + "testing" + + "github.com/helloodokai/acig/internal/diff" + "github.com/helloodokai/acig/internal/verdict" + "github.com/stretchr/testify/require" +) + +func TestGroupFindings(t *testing.T) { + findings := []verdict.Finding{ + {File: "a.txt", LineStart: 10, LineEnd: 10, Title: "Issue A"}, + {File: "a.txt", LineStart: 10, LineEnd: 10, Title: "Issue A2"}, + {File: "b.txt", LineStart: 20, LineEnd: 20, Title: "Issue B"}, + {File: "a.txt", LineStart: 30, LineEnd: 30, Title: "Issue A3"}, + } + + groups := groupFindings(findings) + require.Len(t, groups, 3) + + require.Len(t, groups[0], 2) + require.Equal(t, "a.txt", groups[0][0].File) + require.Equal(t, 10, groups[0][0].LineStart) + + require.Len(t, groups[1], 1) + require.Equal(t, "b.txt", groups[1][0].File) + + require.Len(t, groups[2], 1) + require.Equal(t, 30, groups[2][0].LineStart) +} + +func TestGroupFindings_EmptyFile(t *testing.T) { + findings := []verdict.Finding{ + {File: "", LineStart: 10, Title: "No file"}, + {File: "a.txt", LineStart: 20, Title: "Has file"}, + } + + groups := groupFindings(findings) + require.Len(t, groups, 1) + require.Equal(t, "a.txt", groups[0][0].File) +} + +func TestGroupFindings_ZeroLineStart(t *testing.T) { + findings := []verdict.Finding{ + {File: "a.txt", LineStart: 0, Title: "Zero line"}, + {File: "a.txt", LineStart: 0, Title: "Another zero line"}, + {File: "b.txt", LineStart: 10, Title: "Valid line"}, + } + + groups := groupFindings(findings) + require.Len(t, groups, 2) + require.Equal(t, "a.txt", groups[0][0].File) + require.Len(t, groups[0], 2) + require.Equal(t, "b.txt", groups[1][0].File) +} + +// hr is a helper to create HunkRange slices +func hr(ranges ...diff.HunkRange) []diff.HunkRange { + return ranges +} + +func TestBuildReviewComments_FiltersNonPRFiles(t *testing.T) { + v := &verdict.Verdict{ + Findings: []verdict.Finding{ + {File: "a.txt", LineStart: 10, Title: "Issue A"}, + {File: "deleted.txt", LineStart: 20, Title: "Issue in deleted file"}, + {File: "b.txt", LineStart: 30, Title: "Issue B"}, + }, + } + + prFiles := []string{"a.txt", "b.txt", "c.txt"} + fileDiffs := map[string]*diff.FileDiff{ + "a.txt": {Path: "a.txt", StartLine: 5, HunkRanges: hr(diff.HunkRange{Start: 5, End: 15}), Added: []string{"l5", "l6", "l7", "l8", "l9", "l10"}}, + "b.txt": {Path: "b.txt", StartLine: 25, HunkRanges: hr(diff.HunkRange{Start: 25, End: 35}), Added: []string{"l25", "l26", "l27", "l28", "l29", "l30"}}, + } + comments := buildReviewComments(v, prFiles, fileDiffs) + + require.Len(t, comments, 2) + require.Equal(t, "a.txt", comments[0].Path) + require.Equal(t, "b.txt", comments[1].Path) +} + +func TestBuildReviewComments_NoCommentsWhenFileDiffsNil(t *testing.T) { + v := &verdict.Verdict{ + Findings: []verdict.Finding{ + {File: "a.txt", LineStart: 10, Title: "Issue A"}, + {File: "deleted.txt", LineStart: 20, Title: "Issue in deleted file"}, + }, + } + + comments := buildReviewComments(v, nil, nil) + + require.Len(t, comments, 0) +} + +func TestBuildReviewComments_EmptyPRFiles(t *testing.T) { + v := &verdict.Verdict{ + Findings: []verdict.Finding{ + {File: "a.txt", LineStart: 5, Title: "Issue A"}, + }, + } + + fileDiffs := map[string]*diff.FileDiff{ + "a.txt": {Path: "a.txt", StartLine: 1, HunkRanges: hr(diff.HunkRange{Start: 1, End: 10}), Added: []string{"l1", "l2", "l3", "l4", "l5"}}, + } + comments := buildReviewComments(v, []string{}, fileDiffs) + + require.Len(t, comments, 1) +} + +func TestBuildReviewComments_MultiLineComment(t *testing.T) { + v := &verdict.Verdict{ + Findings: []verdict.Finding{ + {File: "a.txt", LineStart: 10, LineEnd: 20, Title: "Multi-line issue"}, + }, + } + + fileDiffs := map[string]*diff.FileDiff{ + "a.txt": {Path: "a.txt", StartLine: 1, HunkRanges: hr(diff.HunkRange{Start: 1, End: 30}), Added: make([]string, 30)}, + } + comments := buildReviewComments(v, []string{"a.txt"}, fileDiffs) + + require.Len(t, comments, 1) + require.Equal(t, 20, comments[0].Line) + require.Equal(t, 10, comments[0].StartLine) +} + +func TestBuildReviewComments_SingleLineComment(t *testing.T) { + v := &verdict.Verdict{ + Findings: []verdict.Finding{ + {File: "a.txt", LineStart: 10, LineEnd: 10, Title: "Single line issue"}, + }, + } + + fileDiffs := map[string]*diff.FileDiff{ + "a.txt": {Path: "a.txt", StartLine: 1, HunkRanges: hr(diff.HunkRange{Start: 1, End: 20}), Added: make([]string, 20)}, + } + comments := buildReviewComments(v, []string{"a.txt"}, fileDiffs) + + require.Len(t, comments, 1) + require.Equal(t, 10, comments[0].Line) + require.Equal(t, 0, comments[0].StartLine) +} + +func TestBuildReviewComments_GroupsFindingsByFileAndLine(t *testing.T) { + v := &verdict.Verdict{ + Findings: []verdict.Finding{ + {File: "a.txt", LineStart: 10, Title: "Issue 1"}, + {File: "a.txt", LineStart: 10, Title: "Issue 2"}, + {File: "a.txt", LineStart: 30, Title: "Issue 3"}, + {File: "b.txt", LineStart: 20, Title: "Issue 4"}, + }, + } + + fileDiffs := map[string]*diff.FileDiff{ + "a.txt": {Path: "a.txt", StartLine: 1, HunkRanges: hr(diff.HunkRange{Start: 1, End: 50}), Added: make([]string, 50)}, + "b.txt": {Path: "b.txt", StartLine: 1, HunkRanges: hr(diff.HunkRange{Start: 1, End: 30}), Added: make([]string, 30)}, + } + comments := buildReviewComments(v, []string{"a.txt", "b.txt"}, fileDiffs) + + require.Len(t, comments, 3) + require.Contains(t, comments[0].Body, "Issue 1") + require.Contains(t, comments[0].Body, "Issue 2") +} + +func TestValidateLine(t *testing.T) { + tests := []struct { + name string + fd *diff.FileDiff + requestedLine int + expected int + }{ + { + name: "within hunk range", + fd: &diff.FileDiff{StartLine: 50, HunkRanges: hr(diff.HunkRange{Start: 50, End: 55}), Added: []string{"l1", "l2"}}, + requestedLine: 51, + expected: 51, + }, + { + name: "at hunk start", + fd: &diff.FileDiff{StartLine: 50, HunkRanges: hr(diff.HunkRange{Start: 50, End: 55}), Added: []string{"l1", "l2"}}, + requestedLine: 50, + expected: 50, + }, + { + name: "context line within hunk range", + fd: &diff.FileDiff{StartLine: 50, HunkRanges: hr(diff.HunkRange{Start: 50, End: 55}), Added: []string{"l1", "l2"}}, + requestedLine: 53, + expected: 53, + }, + { + name: "at hunk end", + fd: &diff.FileDiff{StartLine: 50, HunkRanges: hr(diff.HunkRange{Start: 50, End: 55}), Added: []string{"l1", "l2"}}, + requestedLine: 55, + expected: 55, + }, + { + name: "beyond hunk range clamps to nearest start", + fd: &diff.FileDiff{StartLine: 50, HunkRanges: hr(diff.HunkRange{Start: 50, End: 55}), Added: []string{"l1", "l2"}}, + requestedLine: 100, + expected: 50, + }, + { + name: "before hunk start clamps to start", + fd: &diff.FileDiff{StartLine: 50, HunkRanges: hr(diff.HunkRange{Start: 50, End: 55}), Added: []string{"l1", "l2"}}, + requestedLine: 10, + expected: 50, + }, + { + name: "nil file diff returns 0", + fd: nil, + requestedLine: 10, + expected: 0, + }, + { + name: "deleted file returns 0", + fd: &diff.FileDiff{StartLine: 1, IsDelete: true, HunkRanges: hr(diff.HunkRange{Start: 1, End: 10}), Added: []string{"l1"}}, + requestedLine: 1, + expected: 0, + }, + { + name: "empty hunk ranges returns 0", + fd: &diff.FileDiff{StartLine: 1, HunkRanges: nil, Added: []string{"l1"}}, + requestedLine: 1, + expected: 0, + }, + { + name: "line in second hunk range", + fd: &diff.FileDiff{StartLine: 10, HunkRanges: hr(diff.HunkRange{Start: 10, End: 20}, diff.HunkRange{Start: 50, End: 60}), Added: []string{"l1"}}, + requestedLine: 55, + expected: 55, + }, + { + name: "line between hunk ranges clamps to nearest", + fd: &diff.FileDiff{StartLine: 10, HunkRanges: hr(diff.HunkRange{Start: 10, End: 20}, diff.HunkRange{Start: 50, End: 60}), Added: []string{"l1"}}, + requestedLine: 35, + expected: 50, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.fd != nil && tt.fd.HunkRanges != nil { + result := validateLine(tt.fd, tt.requestedLine) + require.Equal(t, tt.expected, result) + } else { + result := validateLine(tt.fd, tt.requestedLine) + require.Equal(t, tt.expected, result) + } + }) + } +} + +func TestBuildReviewComments_LineInHunkRange(t *testing.T) { + v := &verdict.Verdict{ + Findings: []verdict.Finding{ + {File: "app.go", LineStart: 52, LineEnd: 52, Title: "Issue at line 52"}, + }, + } + + fileDiffs := map[string]*diff.FileDiff{ + "app.go": {Path: "app.go", StartLine: 50, HunkRanges: hr(diff.HunkRange{Start: 50, End: 55}), Added: []string{"l50", "l51", "l52", "l53", "l54"}}, + } + comments := buildReviewComments(v, []string{"app.go"}, fileDiffs) + + require.Len(t, comments, 1) + require.Equal(t, "app.go", comments[0].Path) + require.Equal(t, 52, comments[0].Line) +} + +func TestBuildReviewComments_ContextLineInRange(t *testing.T) { + v := &verdict.Verdict{ + Findings: []verdict.Finding{ + {File: "app.go", LineStart: 51, LineEnd: 51, Title: "Issue on context line"}, + }, + } + + fileDiffs := map[string]*diff.FileDiff{ + "app.go": {Path: "app.go", StartLine: 50, HunkRanges: hr(diff.HunkRange{Start: 50, End: 55}), Added: []string{"l52", "l53"}}, + } + comments := buildReviewComments(v, []string{"app.go"}, fileDiffs) + + require.Len(t, comments, 1) + require.Equal(t, 51, comments[0].Line) +} + +func TestBuildReviewComments_SkipsFileNotInFileDiffs(t *testing.T) { + v := &verdict.Verdict{ + Findings: []verdict.Finding{ + {File: "missing.go", LineStart: 10, Title: "File not in diff"}, + {File: "present.go", LineStart: 10, Title: "File in diff"}, + }, + } + + fileDiffs := map[string]*diff.FileDiff{ + "present.go": {Path: "present.go", StartLine: 1, HunkRanges: hr(diff.HunkRange{Start: 1, End: 20}), Added: []string{"l1", "l2", "l3", "l4", "l5", "l6", "l7", "l8", "l9", "l10"}}, + } + comments := buildReviewComments(v, []string{"missing.go", "present.go"}, fileDiffs) + + require.Len(t, comments, 1) + require.Equal(t, "present.go", comments[0].Path) +} + +func TestBuildReviewComments_ClampsOutOfRangeLine(t *testing.T) { + v := &verdict.Verdict{ + Findings: []verdict.Finding{ + {File: "a.txt", LineStart: 500, Title: "Way out of range"}, + }, + } + + fileDiffs := map[string]*diff.FileDiff{ + "a.txt": {Path: "a.txt", StartLine: 10, HunkRanges: hr(diff.HunkRange{Start: 10, End: 15}), Added: []string{"l10", "l11", "l12"}}, + } + comments := buildReviewComments(v, []string{"a.txt"}, fileDiffs) + + require.Len(t, comments, 1) + require.Equal(t, 10, comments[0].Line) +} + +func TestBuildReviewComments_LineInSecondHunkRange(t *testing.T) { + v := &verdict.Verdict{ + Findings: []verdict.Finding{ + {File: "multi.go", LineStart: 55, Title: "Issue in second hunk"}, + }, + } + + fileDiffs := map[string]*diff.FileDiff{ + "multi.go": {Path: "multi.go", StartLine: 10, HunkRanges: hr(diff.HunkRange{Start: 10, End: 20}, diff.HunkRange{Start: 50, End: 60}), Added: []string{"l1", "l2"}}, + } + comments := buildReviewComments(v, []string{"multi.go"}, fileDiffs) + + require.Len(t, comments, 1) + require.Equal(t, "multi.go", comments[0].Path) + require.Equal(t, 55, comments[0].Line) +} + +func TestBuildReviewComments_ZeroLineStartClampsToHunkStart(t *testing.T) { + v := &verdict.Verdict{ + Findings: []verdict.Finding{ + {File: "app.ts", LineStart: 0, Title: "Issue with no line"}, + {File: "app.ts", LineStart: 0, Title: "Another issue with no line"}, + }, + } + + fileDiffs := map[string]*diff.FileDiff{ + "app.ts": {Path: "app.ts", StartLine: 5, HunkRanges: hr(diff.HunkRange{Start: 5, End: 15}), Added: []string{"l5", "l6"}}, + } + comments := buildReviewComments(v, []string{"app.ts"}, fileDiffs) + + require.Len(t, comments, 1) + require.Equal(t, "app.ts", comments[0].Path) + require.Equal(t, 5, comments[0].Line) + require.Contains(t, comments[0].Body, "2 issue(s)") +} \ No newline at end of file diff --git a/internal/reporters/markdown.go b/internal/reporters/markdown.go index 5b8846f..92d5aa9 100644 --- a/internal/reporters/markdown.go +++ b/internal/reporters/markdown.go @@ -30,40 +30,31 @@ func WriteMarkdown(v *verdict.Verdict, path string) error { func FormatMarkdown(v *verdict.Verdict) string { var sb strings.Builder - sb.WriteString(fmt.Sprintf("# Acig Verdict: %s\n\n", strings.ToUpper(string(v.Decision)))) + decisionEmoji := "✅" + switch v.Decision { + case verdict.DecisionWarn: + decisionEmoji = "⚠ī¸" + case verdict.DecisionBlock: + decisionEmoji = "đŸšĢ" + } + sb.WriteString(fmt.Sprintf("# %s Acig Verdict: %s\n\n", decisionEmoji, strings.ToUpper(string(v.Decision)))) sb.WriteString(fmt.Sprintf("**Risk:** %s | **Cost:** $%.4f | **Duration:** %dms\n\n", v.Risk, v.TotalCostUSD, v.TotalDurationMS)) sb.WriteString(fmt.Sprintf("%s\n\n", v.Summary)) - if len(v.Findings) == 0 { + totalFindings := len(v.Findings) + len(v.DanglingFindings) + if totalFindings == 0 { sb.WriteString("No findings. Code looks clean.\n") } else { sb.WriteString("## Findings\n\n") for _, f := range v.Findings { - severity := string(f.Severity) - switch f.Severity { - case verdict.SeverityBlocking: - severity = "đŸšĢ BLOCKING" - case verdict.SeverityHigh: - severity = "🔴 HIGH" - case verdict.SeverityMedium: - severity = "🟡 MEDIUM" - case verdict.SeverityLow: - severity = "đŸŸĸ LOW" - case verdict.SeverityInfo: - severity = "ℹī¸ INFO" - } - sb.WriteString(fmt.Sprintf("### [%s] %s\n", severity, f.Title)) - sb.WriteString(fmt.Sprintf("- **Critic:** %s\n", f.Critic)) - if f.File != "" { - loc := f.File - if f.LineStart > 0 { - loc += fmt.Sprintf(":%d", f.LineStart) - } - sb.WriteString(fmt.Sprintf("- **Location:** %s\n", loc)) - } - sb.WriteString(fmt.Sprintf("\n%s\n\n", f.Detail)) - if f.SuggestedFix != "" { - sb.WriteString(fmt.Sprintf("> **Suggested fix:** %s\n\n", f.SuggestedFix)) + sb.WriteString(formatFinding(f)) + } + + if len(v.DanglingFindings) > 0 { + sb.WriteString("## Observations (files not in this PR)\n\n") + sb.WriteString("_The following findings reference files not changed in this PR._\n\n") + for _, f := range v.DanglingFindings { + sb.WriteString(formatDanglingFinding(f)) } } } @@ -86,4 +77,55 @@ func FormatMarkdown(v *verdict.Verdict) string { sb.WriteString(fmt.Sprintf("---\n*Generated by [acig](https://github.com/helloodokai/acig) â€ĸ SHA: %s*\n", v.SHA)) return sb.String() +} + +func formatFinding(f verdict.Finding) string { + var sb strings.Builder + severity := severityEmojiMarkdown(f.Severity) + sb.WriteString(fmt.Sprintf("### [%s] %s\n", severity, f.Title)) + sb.WriteString(fmt.Sprintf("- **Critic:** %s\n", f.Critic)) + if f.File != "" { + loc := f.File + if f.LineStart > 0 { + loc += fmt.Sprintf(":%d", f.LineStart) + } + sb.WriteString(fmt.Sprintf("- **Location:** `%s`\n", loc)) + } + sb.WriteString(fmt.Sprintf("\n%s\n\n", f.Detail)) + if f.SuggestedFix != "" { + sb.WriteString(fmt.Sprintf("> **Suggested fix:** %s\n\n", f.SuggestedFix)) + } + return sb.String() +} + +func formatDanglingFinding(f verdict.Finding) string { + var sb strings.Builder + severity := severityEmojiMarkdown(f.Severity) + sb.WriteString(fmt.Sprintf("#### [%s] %s\n", severity, f.Title)) + sb.WriteString(fmt.Sprintf("- **Critic:** %s\n", f.Critic)) + if f.File != "" { + sb.WriteString(fmt.Sprintf("- **File:** `%s`\n", f.File)) + } + sb.WriteString(fmt.Sprintf("\n%s\n\n", f.Detail)) + if f.SuggestedFix != "" { + sb.WriteString(fmt.Sprintf("> **Suggested fix:** %s\n\n", f.SuggestedFix)) + } + return sb.String() +} + +func severityEmojiMarkdown(s verdict.Severity) string { + switch s { + case verdict.SeverityBlocking: + return "đŸšĢ BLOCKING" + case verdict.SeverityHigh: + return "🔴 HIGH" + case verdict.SeverityMedium: + return "🟡 MEDIUM" + case verdict.SeverityLow: + return "đŸŸĸ LOW" + case verdict.SeverityInfo: + return "ℹī¸ INFO" + default: + return string(s) + } } \ No newline at end of file diff --git a/internal/verdict/validate_paths.go b/internal/verdict/validate_paths.go new file mode 100644 index 0000000..8fa76dc --- /dev/null +++ b/internal/verdict/validate_paths.go @@ -0,0 +1,230 @@ +package verdict + +import "log/slog" + +// CategorizeFindingsByPath splits findings into three groups based on whether +// their File field appears in the diff paths: +// - inDiff: findings whose file is in the diff (can be inline-commented) +// - dangling: findings whose file is NOT in the diff but is a plausible +// suggestion (e.g. a test file that should exist alongside a changed source file) +// - hallucinated: findings whose file appears fabricated (no clear relationship +// to any file in the diff and the file is not a common test/config variant) +// +// In-diff findings are always kept. Dangling findings are kept but should NOT be +// posted as inline review comments (GitHub will reject them). Hallucinated +// findings are dropped entirely with a warning log. +func CategorizeFindingsByPath(findings []Finding, diffPaths []string) (inDiff, dangling, hallucinated []Finding) { + if len(diffPaths) == 0 { + return findings, nil, nil + } + + valid := make(map[string]bool, len(diffPaths)) + for _, p := range diffPaths { + valid[p] = true + } + + for _, f := range findings { + if f.File == "" { + inDiff = append(inDiff, f) + continue + } + if valid[f.File] { + inDiff = append(inDiff, f) + continue + } + + if isPlausibleSuggestion(f.File, diffPaths) { + dangling = append(dangling, f) + continue + } + + slog.Warn("filtering hallucinated file path from finding", + "critic", f.Critic, + "file", f.File, + "line_start", f.LineStart, + "title", f.Title, + ) + hallucinated = append(hallucinated, f) + } + return inDiff, dangling, hallucinated +} + +// isPlausibleSuggestion checks whether a file path that doesn't appear in the +// diff could be a legitimate suggestion (e.g. a test file that should exist +// alongside a changed source file, or a migration/config that pairs with a +// change). It returns true if: +// - The basename of the suggested file matches or is a test variant of a +// file that IS in the diff +// - The suggested file is in the same directory tree as a diff file AND +// looks like a complementary file type +// - The suggested file looks like a complementary file regardless of +// directory (test files, migrations in common locations) +func isPlausibleSuggestion(filePath string, diffPaths []string) bool { + for _, dp := range diffPaths { + if isTestVariant(filePath, dp) { + return true + } + if sameDirectoryTree(filePath, dp) && looksLikeComplementaryFile(filePath) { + return true + } + } + // Files that look like complementary types regardless of directory + if looksLikeComplementaryFile(filePath) { + return true + } + return false +} + +// isTestVariant checks if filePath is a test/spec variant of diffPath. +// e.g. plan-tools.test.ts is a test variant of plan-tools.ts +// PlanTools.spec.tsx is a test variant of PlanTools.tsx +// Returns false if filePath equals diffPath (same file, not a variant). +func isTestVariant(filePath, diffPath string) bool { + if filePath == diffPath { + return false + } + + fb := filepathBase(filePath) + db := filepathBase(diffPath) + + fCore := stripAllExtensions(fb) + dCore := stripAllExtensions(db) + + fstripped := stripTestInfix(fCore) + + return fstripped == dCore +} + +// stripAllExtensions removes all extensions from a filename. +// e.g. "plan-tools.test.ts" -> "plan-tools" +// e.g. "PlanTools.spec.tsx" -> "PlanTools" +// e.g. "handler.go" -> "handler" +func stripAllExtensions(name string) string { + for i := 0; i < len(name); i++ { + if name[i] == '.' { + return name[:i] + } + } + return name +} + +// stripTestInfix removes test/spec infixes from a filename core. +// e.g. "plan-tools_test" -> "plan-tools" +// e.g. "PlanTools.spec" -> "PlanTools" +// e.g. "plan-tools-test" -> "plan-tools" +func stripTestInfix(s string) string { + for _, infix := range []string{"_test", "_spec", ".test", ".spec", "-test", "-spec"} { + if idx := indexOf(s, infix); idx != -1 { + return s[:idx] + } + } + return s +} + +func indexOf(s, substr string) int { + for i := 0; i <= len(s)-len(substr); i++ { + if s[i:i+len(substr)] == substr { + return i + } + } + return -1 +} + +func filepathBase(path string) string { + for i := len(path) - 1; i >= 0; i-- { + if path[i] == '/' { + return path[i+1:] + } + } + return path +} + +// sameDirectoryTree checks if two paths share a common parent directory. +func sameDirectoryTree(a, b string) bool { + aDir := dirOf(a) + bDir := dirOf(b) + if aDir == "" || bDir == "" { + return false + } + return aDir == bDir +} + +func dirOf(path string) string { + for i := len(path) - 1; i >= 0; i-- { + if path[i] == '/' { + return path[:i] + } + } + return "" +} + +// looksLikeComplementaryFile returns true if the file path looks like a +// test, spec, migration, or config file that would logically accompany +// a source file in the diff. +func looksLikeComplementaryFile(path string) bool { + lower := toLower(path) + // Test/spec files (compound extensions like .test.ts, .spec.tsx) + if contains(lower, ".test.") || contains(lower, ".spec.") || + contains(lower, "_test.") || contains(lower, "_spec.") { + return true + } + // Test directories + if contains(lower, "/test/") || contains(lower, "/tests/") || + contains(lower, "/__tests__/") || contains(lower, "/spec/") { + return true + } + // Also match paths starting with test directories + if hasPrefix(lower, "test/") || hasPrefix(lower, "tests/") || + hasPrefix(lower, "spec/") || hasPrefix(lower, "__tests__/") { + return true + } + // Migration directories and files + if contains(lower, "/migrations/") || contains(lower, "/migration/") || + contains(lower, "/db/") || contains(lower, "/migrate/") { + return true + } + if hasPrefix(lower, "migrations/") || hasPrefix(lower, "migration/") { + return true + } + // Config files (ending in .config.ts, .config.js, .rc.js, etc.) + base := filepathBase(lower) + if contains(base, ".config.") || contains(base, ".rc.") { + return true + } + return false +} + +func toLower(s string) string { + result := make([]byte, len(s)) + for i := range s { + c := s[i] + if c >= 'A' && c <= 'Z' { + c += 'a' - 'A' + } + result[i] = c + } + return string(result) +} + +func contains(s, substr string) bool { + for i := 0; i+len(substr) <= len(s); i++ { + if s[i:i+len(substr)] == substr { + return true + } + } + return false +} + +func hasSuffix(s, suffix string) bool { + if len(suffix) > len(s) { + return false + } + return s[len(s)-len(suffix):] == suffix +} + +func hasPrefix(s, prefix string) bool { + if len(prefix) > len(s) { + return false + } + return s[:len(prefix)] == prefix +} \ No newline at end of file diff --git a/internal/verdict/validate_paths_test.go b/internal/verdict/validate_paths_test.go new file mode 100644 index 0000000..d23a3ba --- /dev/null +++ b/internal/verdict/validate_paths_test.go @@ -0,0 +1,185 @@ +package verdict + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestCategorizeFindingsByPath_KeepsDiffFiles(t *testing.T) { + findings := []Finding{ + {File: "apps/backend/src/lib/tools/plan-tools.ts", LineStart: 10, Title: "real issue"}, + {File: "src/main.ts", LineStart: 5, Title: "another real issue"}, + } + diffPaths := []string{"apps/backend/src/lib/tools/plan-tools.ts", "src/main.ts", "README.md"} + + inDiff, dangling, hallucinated := CategorizeFindingsByPath(findings, diffPaths) + require.Len(t, inDiff, 2) + require.Empty(t, dangling) + require.Empty(t, hallucinated) +} + +func TestCategorizeFindingsByPath_HallucinatedFiles(t *testing.T) { + findings := []Finding{ + {File: "service/get_plan_context.go", LineStart: 10, Critic: "perf_smell", Title: "hallucinated Go file"}, + {File: "src/services/planContextService.js", LineStart: 5, Critic: "test_coverage_smell", Title: "hallucinated JS file"}, + {File: "apps/backend/src/lib/tools/plan-tools.ts", LineStart: 20, Critic: "risk_classifier", Title: "real file"}, + } + diffPaths := []string{"apps/backend/src/lib/tools/plan-tools.ts", ".charters/ch-2026-05-07-12f7a9.spec.md"} + + inDiff, dangling, hallucinated := CategorizeFindingsByPath(findings, diffPaths) + require.Len(t, inDiff, 1) + require.Equal(t, "apps/backend/src/lib/tools/plan-tools.ts", inDiff[0].File) + require.Empty(t, dangling) + require.Len(t, hallucinated, 2) +} + +func TestCategorizeFindingsByPath_TestFileSuggestionsAreDangling(t *testing.T) { + findings := []Finding{ + {File: "apps/backend/src/lib/tools/plan-tools.test.ts", LineStart: 0, Critic: "test_coverage_smell", Title: "Missing test for empty plan context"}, + {File: "apps/backend/src/lib/tools/plan-tools.ts", LineStart: 42, Critic: "risk_classifier", Title: "SQL injection"}, + } + diffPaths := []string{"apps/backend/src/lib/tools/plan-tools.ts", ".charters/ch-2026-05-07-12f7a9.spec.md"} + + inDiff, dangling, hallucinated := CategorizeFindingsByPath(findings, diffPaths) + require.Len(t, inDiff, 1) + require.Equal(t, "apps/backend/src/lib/tools/plan-tools.ts", inDiff[0].File) + require.Len(t, dangling, 1) + require.Equal(t, "apps/backend/src/lib/tools/plan-tools.test.ts", dangling[0].File) + require.Empty(t, hallucinated) +} + +func TestCategorizeFindingsByPath_SpecFileSuggestionsAreDangling(t *testing.T) { + findings := []Finding{ + {File: "src/component.spec.tsx", LineStart: 0, Critic: "test_coverage_smell", Title: "Missing spec"}, + {File: "tests/unit/migrations/20240507_add_plan_context_index.sql", LineStart: 0, Critic: "test_coverage_smell", Title: "Missing migration test"}, + } + diffPaths := []string{"src/component.tsx", "db/migrations/20240507.sql"} + + inDiff, dangling, hallucinated := CategorizeFindingsByPath(findings, diffPaths) + require.Len(t, inDiff, 0) + require.Len(t, dangling, 2) + require.Empty(t, hallucinated) +} + +func TestCategorizeFindingsByPath_EmptyFileFindingsAreInDiff(t *testing.T) { + findings := []Finding{ + {File: "", LineStart: 0, Title: "general finding no file"}, + {File: "real.ts", LineStart: 1, Title: "file finding"}, + } + diffPaths := []string{"real.ts"} + + inDiff, dangling, hallucinated := CategorizeFindingsByPath(findings, diffPaths) + require.Len(t, inDiff, 2) + require.Empty(t, dangling) + require.Empty(t, hallucinated) +} + +func TestCategorizeFindingsByPath_EmptyDiffPathsKeepsAll(t *testing.T) { + findings := []Finding{ + {File: "madeup.go", LineStart: 10, Title: "hallucinated"}, + {File: "also_madeup.js", LineStart: 20, Title: "also hallucinated"}, + } + + inDiff, _, _ := CategorizeFindingsByPath(findings, nil) + require.Len(t, inDiff, 2) + + inDiff, dangling, hallucinated := CategorizeFindingsByPath(findings, []string{}) + require.Len(t, inDiff, 2) + require.Empty(t, dangling) + require.Empty(t, hallucinated) +} + +func TestCategorizeFindingsByPath_AllHallucinated(t *testing.T) { + findings := []Finding{ + {File: "service/get_plan_context.go", LineStart: 1, Critic: "perf_smell", Title: "hallucination 1"}, + {File: "repository/plan_context_repository.go", LineStart: 5, Critic: "perf_smell", Title: "hallucination 2"}, + {File: "handlers/context_handler.go", LineStart: 10, Critic: "test_coverage_smell", Title: "hallucination 3"}, + } + diffPaths := []string{".charters/ch-2026-05-07-12f7a9.spec.md"} + + inDiff, dangling, hallucinated := CategorizeFindingsByPath(findings, diffPaths) + require.Empty(t, inDiff) + require.Empty(t, dangling) + require.Len(t, hallucinated, 3) +} + +func TestCategorizeFindingsByPath_NilFindings(t *testing.T) { + inDiff, dangling, hallucinated := CategorizeFindingsByPath(nil, []string{"a.txt"}) + require.Len(t, inDiff, 0) + require.Empty(t, dangling) + require.Empty(t, hallucinated) +} + +func TestCategorizeFindingsByPath_PreservesFindingFields(t *testing.T) { + findings := []Finding{ + { + Critic: "risk_classifier", + Severity: SeverityHigh, + Title: "SQL injection", + Detail: "dangerous query construction", + File: "src/db.ts", + LineStart: 42, + LineEnd: 45, + SuggestedFix: "use parameterized query", + }, + } + diffPaths := []string{"src/db.ts"} + + inDiff, _, _ := CategorizeFindingsByPath(findings, diffPaths) + require.Len(t, inDiff, 1) + require.Equal(t, "risk_classifier", inDiff[0].Critic) + require.Equal(t, SeverityHigh, inDiff[0].Severity) + require.Equal(t, "SQL injection", inDiff[0].Title) + require.Equal(t, "src/db.ts", inDiff[0].File) + require.Equal(t, 42, inDiff[0].LineStart) + require.Equal(t, 45, inDiff[0].LineEnd) + require.Equal(t, "use parameterized query", inDiff[0].SuggestedFix) +} + +func TestIsTestVariant(t *testing.T) { + tests := []struct { + filePath string + diffPath string + want bool + }{ + {"plan-tools.test.ts", "plan-tools.ts", true}, + {"app.test.ts", "app.ts", true}, + {"Component.spec.tsx", "Component.tsx", true}, + {"handler_test.go", "handler.go", true}, + {"service_spec.py", "service.py", true}, + {"plan-tools.ts", "plan-tools.ts", false}, // same file, not a test variant + {"unrelated.ts", "plan-tools.ts", false}, + {"app.ts", "plan-tools.test.ts", false}, // reversed, not a test variant + } + + for _, tt := range tests { + t.Run(tt.filePath+"_"+tt.diffPath, func(t *testing.T) { + got := isTestVariant(tt.filePath, tt.diffPath) + require.Equal(t, tt.want, got) + }) + } +} + +func TestLooksLikeComplementaryFile(t *testing.T) { + tests := []struct { + path string + want bool + }{ + {"src/app.test.ts", true}, + {"src/app.spec.tsx", true}, + {"test/unit/handler_test.go", true}, + {"__tests__/component.test.tsx", true}, + {"migrations/20240507_add_index.sql", true}, + {"src/app.config.ts", true}, + {"src/main.ts", false}, + {"service/get_plan_context.go", false}, + } + + for _, tt := range tests { + t.Run(tt.path, func(t *testing.T) { + got := looksLikeComplementaryFile(tt.path) + require.Equal(t, tt.want, got) + }) + } +} \ No newline at end of file diff --git a/internal/verdict/verdict.go b/internal/verdict/verdict.go index 7404fd7..6ede3e1 100644 --- a/internal/verdict/verdict.go +++ b/internal/verdict/verdict.go @@ -60,6 +60,7 @@ type Verdict struct { Decision Decision `json:"decision"` Summary string `json:"summary"` Findings []Finding `json:"findings"` + DanglingFindings []Finding `json:"dangling_findings,omitempty"` CriticResults []CriticResult `json:"critic_results"` TotalCostUSD float64 `json:"total_cost_usd"` TotalDurationMS int64 `json:"total_duration_ms"` diff --git a/internal/version/version.go b/internal/version/version.go index 1f92cb5..e3063ff 100644 --- a/internal/version/version.go +++ b/internal/version/version.go @@ -1,3 +1,3 @@ package version -var Version = "dev" \ No newline at end of file +var Version = "1.8.0" \ No newline at end of file