From 276fe8fb6905487345c998e37d2914d5bf1fe91e Mon Sep 17 00:00:00 2001 From: Lewis Barclay Date: Thu, 7 May 2026 21:12:24 +0100 Subject: [PATCH 01/21] bump: version to v1.5.2 --- internal/version/version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/version/version.go b/internal/version/version.go index 1f92cb5..d869262 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.5.2" \ No newline at end of file From f8bc123fbccbbea4da2c376254e1e11a941ec5b1 Mon Sep 17 00:00:00 2001 From: Lewis Barclay Date: Thu, 7 May 2026 23:46:54 +0100 Subject: [PATCH 02/21] fix: filter review comments to only files in PR diff Prevent 422 'Path could not be resolved' errors when creating review comments on files that don't exist in the PR's diff (e.g., deleted files). - Add ListPRFiles() to fetch PR's changed files - Filter buildReviewComments() to only include files present in PR diff - Add tests for groupFindings and buildReviewComments --- internal/githubclient/review.go | 12 +++ internal/githubclient/review_test.go | 40 ++++++++ internal/reporters/github.go | 18 +++- internal/reporters/github_test.go | 139 +++++++++++++++++++++++++++ 4 files changed, 207 insertions(+), 2 deletions(-) create mode 100644 internal/githubclient/review_test.go create mode 100644 internal/reporters/github_test.go diff --git a/internal/githubclient/review.go b/internal/githubclient/review.go index c872a53..fb0a14e 100644 --- a/internal/githubclient/review.go +++ b/internal/githubclient/review.go @@ -99,4 +99,16 @@ 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 } \ 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..694d967 --- /dev/null +++ b/internal/githubclient/review_test.go @@ -0,0 +1,40 @@ +package githubclient + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestReviewCommentStruct(t *testing.T) { + rc := ReviewComment{ + Path: "test.txt", + Line: 10, + StartLine: 5, + Body: "test body", + } + require.Equal(t, "test.txt", rc.Path) + require.Equal(t, 10, rc.Line) + require.Equal(t, 5, rc.StartLine) + require.Equal(t, "test body", rc.Body) +} + +func TestReviewCommentStruct_NoStartLine(t *testing.T) { + rc := ReviewComment{ + Path: "test.txt", + Line: 10, + Body: "test body", + } + require.Equal(t, 0, rc.StartLine) +} + +func TestReviewCommentStruct_MultiLine(t *testing.T) { + rc := ReviewComment{ + Path: "test.txt", + Line: 20, + StartLine: 10, + Body: "multi-line comment", + } + require.True(t, rc.StartLine > 0) + require.Greater(t, rc.Line, rc.StartLine) +} \ No newline at end of file diff --git a/internal/reporters/github.go b/internal/reporters/github.go index d075248..07b5248 100644 --- a/internal/reporters/github.go +++ b/internal/reporters/github.go @@ -30,7 +30,13 @@ 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 + } + reviewComments := buildReviewComments(v, prFiles) event := "COMMENT" if v.Decision == verdict.DecisionBlock { event = "REQUEST_CHANGES" @@ -120,14 +126,22 @@ func (r *GitHubReporter) buildReviewBody(v *verdict.Verdict) string { return body.String() } -func (r *GitHubReporter) buildReviewComments(v *verdict.Verdict) []githubclient.ReviewComment { +func buildReviewComments(v *verdict.Verdict, prFiles []string) []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 { continue } + if len(prFiles) > 0 && !prFilesSet[findings[0].File] { + continue + } var body strings.Builder body.WriteString(fmt.Sprintf("**acig** found %d issue(s) here:\n\n", len(findings))) for i, f := range findings { diff --git a/internal/reporters/github_test.go b/internal/reporters/github_test.go new file mode 100644 index 0000000..369493c --- /dev/null +++ b/internal/reporters/github_test.go @@ -0,0 +1,139 @@ +package reporters + +import ( + "testing" + + "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: "b.txt", LineStart: 10, Title: "Valid line"}, + } + + groups := groupFindings(findings) + require.Len(t, groups, 1) + require.Equal(t, "b.txt", groups[0][0].File) +} + +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"} + comments := buildReviewComments(v, prFiles) + + require.Len(t, comments, 2) + require.Equal(t, "a.txt", comments[0].Path) + require.Equal(t, "b.txt", comments[1].Path) +} + +func TestBuildReviewComments_AllFilesIfPRFilesNil(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) + + require.Len(t, comments, 2) +} + +func TestBuildReviewComments_EmptyPRFiles(t *testing.T) { + v := &verdict.Verdict{ + Findings: []verdict.Finding{ + {File: "a.txt", LineStart: 10, Title: "Issue A"}, + }, + } + + comments := buildReviewComments(v, []string{}) + + 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"}, + }, + } + + comments := buildReviewComments(v, []string{"a.txt"}) + + 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"}, + }, + } + + comments := buildReviewComments(v, []string{"a.txt"}) + + 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"}, + }, + } + + comments := buildReviewComments(v, []string{"a.txt", "b.txt"}) + + require.Len(t, comments, 3) + require.Contains(t, comments[0].Body, "Issue 1") + require.Contains(t, comments[0].Body, "Issue 2") +} \ No newline at end of file From 6a644760bd1636e724e12cf51aede697ba4b410c Mon Sep 17 00:00:00 2001 From: Lewis Barclay Date: Thu, 7 May 2026 23:49:57 +0100 Subject: [PATCH 03/21] fix: use vars.ACIG_APP_ID instead of secrets for app-id --- .github/workflows/acig.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/acig.yml b/.github/workflows/acig.yml index ef9358c..348c283 100644 --- a/.github/workflows/acig.yml +++ b/.github/workflows/acig.yml @@ -19,9 +19,8 @@ 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 != '' }} with: - app-id: ${{ secrets.ACIG_APP_ID }} + app-id: ${{ vars.ACIG_APP_ID }} private-key: ${{ secrets.ACIG_APP_PRIVATE_KEY }} - name: Install acig From db45c97f8bdb39c5cf01bf905bbfae3edd380940 Mon Sep 17 00:00:00 2001 From: Lewis Barclay Date: Thu, 7 May 2026 23:51:46 +0100 Subject: [PATCH 04/21] fix: read all struct fields to satisfy unusedwrite linter --- internal/githubclient/review_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/githubclient/review_test.go b/internal/githubclient/review_test.go index 694d967..f9550c0 100644 --- a/internal/githubclient/review_test.go +++ b/internal/githubclient/review_test.go @@ -25,6 +25,9 @@ func TestReviewCommentStruct_NoStartLine(t *testing.T) { Line: 10, Body: "test body", } + require.Equal(t, "test.txt", rc.Path) + require.Equal(t, 10, rc.Line) + require.Equal(t, "test body", rc.Body) require.Equal(t, 0, rc.StartLine) } @@ -35,6 +38,8 @@ func TestReviewCommentStruct_MultiLine(t *testing.T) { StartLine: 10, Body: "multi-line comment", } + require.Equal(t, "test.txt", rc.Path) + require.Equal(t, "multi-line comment", rc.Body) require.True(t, rc.StartLine > 0) require.Greater(t, rc.Line, rc.StartLine) } \ No newline at end of file From 51c341cf81d48b1f6209cff7f76efc06ffed5dde Mon Sep 17 00:00:00 2001 From: Lewis Barclay Date: Thu, 7 May 2026 23:52:09 +0100 Subject: [PATCH 05/21] add check target that runs lint and test --- Makefile | 2 ++ 1 file changed, 2 insertions(+) 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 ./... From 72445f406968894b3ecae7307987d0523c1ad862 Mon Sep 17 00:00:00 2001 From: Lewis Barclay Date: Thu, 7 May 2026 23:52:50 +0100 Subject: [PATCH 06/21] fix: restore conditional for app token step --- .github/workflows/acig.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/acig.yml b/.github/workflows/acig.yml index 348c283..370d987 100644 --- a/.github/workflows/acig.yml +++ b/.github/workflows/acig.yml @@ -19,6 +19,7 @@ jobs: - name: Get GitHub App token id: app-token uses: actions/create-github-app-token@v2 + if: ${{ vars.ACIG_APP_ID != '' && secrets.ACIG_APP_PRIVATE_KEY != '' }} with: app-id: ${{ vars.ACIG_APP_ID }} private-key: ${{ secrets.ACIG_APP_PRIVATE_KEY }} From e8130272d20e2198c2f02b3ba320c0d2d690b26f Mon Sep 17 00:00:00 2001 From: Lewis Barclay Date: Thu, 7 May 2026 23:53:44 +0100 Subject: [PATCH 07/21] fix: remove invalid if condition from workflow --- .github/workflows/acig.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/acig.yml b/.github/workflows/acig.yml index 370d987..348c283 100644 --- a/.github/workflows/acig.yml +++ b/.github/workflows/acig.yml @@ -19,7 +19,6 @@ jobs: - name: Get GitHub App token id: app-token uses: actions/create-github-app-token@v2 - if: ${{ vars.ACIG_APP_ID != '' && secrets.ACIG_APP_PRIVATE_KEY != '' }} with: app-id: ${{ vars.ACIG_APP_ID }} private-key: ${{ secrets.ACIG_APP_PRIVATE_KEY }} From 8c53f22578ae28ac3cf17b716509ccb60e3a0401 Mon Sep 17 00:00:00 2001 From: Lewis Barclay Date: Thu, 7 May 2026 23:54:28 +0100 Subject: [PATCH 08/21] fix: skip app token step when app-id is empty --- .github/workflows/acig.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/acig.yml b/.github/workflows/acig.yml index 348c283..2de36e1 100644 --- a/.github/workflows/acig.yml +++ b/.github/workflows/acig.yml @@ -19,6 +19,7 @@ jobs: - name: Get GitHub App token id: app-token uses: actions/create-github-app-token@v2 + if: ${{ vars.ACIG_APP_ID != '' }} with: app-id: ${{ vars.ACIG_APP_ID }} private-key: ${{ secrets.ACIG_APP_PRIVATE_KEY }} From b3bc3ada056bfbf5ac08057ebc443ee1f4cbbf1a Mon Sep 17 00:00:00 2001 From: Lewis Barclay Date: Fri, 8 May 2026 00:05:38 +0100 Subject: [PATCH 09/21] fix: retry review without inline comments on position errors --- internal/reporters/github.go | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/internal/reporters/github.go b/internal/reporters/github.go index 07b5248..fead304 100644 --- a/internal/reporters/github.go +++ b/internal/reporters/github.go @@ -43,18 +43,21 @@ func (r *GitHubReporter) Report(ctx context.Context, v *verdict.Verdict, owner, } 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" From c2c865787a4d554df33ed19bf7dceb643ea0bebf Mon Sep 17 00:00:00 2001 From: Lewis Barclay Date: Fri, 8 May 2026 00:08:40 +0100 Subject: [PATCH 10/21] test: add integration tests for Report with mock GitHub client - TestReport_RetryWithoutCommentsOnPositionError: verifies retry logic - TestReport_FallsBackToStickyCommentWhenBothReviewAttemptsFail - TestReport_SuccessfulReviewOnFirstTry - TestReport_UsesPRFilesFilter Also converts GitHubReporter.client to use interface for testability. --- internal/reporters/github.go | 16 ++- internal/reporters/github_report_test.go | 159 +++++++++++++++++++++++ 2 files changed, 173 insertions(+), 2 deletions(-) create mode 100644 internal/reporters/github_report_test.go diff --git a/internal/reporters/github.go b/internal/reporters/github.go index fead304..16db0c8 100644 --- a/internal/reporters/github.go +++ b/internal/reporters/github.go @@ -7,6 +7,7 @@ import ( "log/slog" "strings" + "github.com/google/go-github/v66/github" "github.com/helloodokai/acig/internal/githubclient" "github.com/helloodokai/acig/internal/verdict" ) @@ -14,11 +15,22 @@ 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) + 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} } diff --git a/internal/reporters/github_report_test.go b/internal/reporters/github_report_test.go new file mode 100644 index 0000000..8acc8c6 --- /dev/null +++ b/internal/reporters/github_report_test.go @@ -0,0 +1,159 @@ +package reporters + +import ( + "context" + "errors" + "testing" + + "github.com/google/go-github/v66/github" + "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 +} + +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) 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 From 9f7962f930d48f7d48d3c2f44eb847d3c6bedd8d Mon Sep 17 00:00:00 2001 From: Lewis Barclay Date: Fri, 8 May 2026 00:12:13 +0100 Subject: [PATCH 11/21] ci: fetch latest ACIG version instead of hardcoding --- .github/workflows/acig.yml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.github/workflows/acig.yml b/.github/workflows/acig.yml index 2de36e1..8842bdf 100644 --- a/.github/workflows/acig.yml +++ b/.github/workflows/acig.yml @@ -28,7 +28,12 @@ jobs: env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | - ACIG_VERSION="1.4.1" + ACIG_VERSION=$(curl -sL "https://api.github.com/repos/helloodokai/acig/releases/latest" | grep '"tag_name"' | sed 's/.*v\([0-9.]*\).*/\1/') + if [ -z "$ACIG_VERSION" ]; then + echo "::error::could not determine latest acig version" + exit 1 + fi + echo "Using ACIG version ${ACIG_VERSION}" RELEASE_URL="https://github.com/helloodokai/acig/releases/download/v${ACIG_VERSION}" curl -sL "${RELEASE_URL}/checksums.txt" -o /tmp/checksums.txt From 37f08d1167ead7cc83e9f082cf5928b3ece09f64 Mon Sep 17 00:00:00 2001 From: Lewis Barclay Date: Fri, 8 May 2026 00:21:51 +0100 Subject: [PATCH 12/21] fix: validate line numbers against PR diff before commenting - Add GetPRFileDiffs() to fetch file diffs with line counts - Add validateLine() to cap line numbers to actual changed lines - buildReviewComments now validates line numbers against actual diff - Skip comments for invalid/outsized line numbers instead of failing This should fix 'Line could not be resolved' errors when AI reports line numbers beyond the actual changes in the diff. --- internal/githubclient/review.go | 26 ++++++++++ internal/reporters/github.go | 61 +++++++++++++++++++++--- internal/reporters/github_report_test.go | 11 +++++ internal/reporters/github_test.go | 12 ++--- 4 files changed, 98 insertions(+), 12 deletions(-) diff --git a/internal/githubclient/review.go b/internal/githubclient/review.go index fb0a14e..a394819 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) { @@ -111,4 +112,29 @@ func (c *Client) ListPRFiles(ctx context.Context, owner, repo string, prNumber i 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 { + fd := &diff.FileDiff{ + Path: f.GetFilename(), + IsNew: f.GetStatus() == "added", + IsDelete: f.GetStatus() == "removed", + } + if f.Patch != nil { + d, parseErr := diff.FromFiles(*f.Patch) + if parseErr == nil && len(d.Files) > 0 { + fd.Added = d.Files[0].Added + fd.Removed = d.Files[0].Removed + } + } + result[f.GetFilename()] = fd + } + return result, nil } \ No newline at end of file diff --git a/internal/reporters/github.go b/internal/reporters/github.go index 16db0c8..05733a3 100644 --- a/internal/reporters/github.go +++ b/internal/reporters/github.go @@ -8,6 +8,7 @@ import ( "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" ) @@ -21,6 +22,7 @@ type GitHubReporter struct { 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 @@ -48,7 +50,14 @@ func (r *GitHubReporter) Report(ctx context.Context, v *verdict.Verdict, owner, slog.Warn("failed to list PR files, using all findings", "error", err) prFiles = nil } - reviewComments := buildReviewComments(v, prFiles) + + 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" @@ -141,7 +150,7 @@ func (r *GitHubReporter) buildReviewBody(v *verdict.Verdict) string { return body.String() } -func buildReviewComments(v *verdict.Verdict, prFiles []string) []githubclient.ReviewComment { +func buildReviewComments(v *verdict.Verdict, prFiles []string, fileDiffs map[string]*diff.FileDiff) []githubclient.ReviewComment { fileFindings := groupFindings(v.Findings) var comments []githubclient.ReviewComment @@ -157,6 +166,32 @@ func buildReviewComments(v *verdict.Verdict, prFiles []string) []githubclient.Re if len(prFiles) > 0 && !prFilesSet[findings[0].File] { continue } + + lineStart := findings[0].LineStart + lineEnd := findings[0].LineEnd + + if fileDiffs != nil { + fd := fileDiffs[findings[0].File] + if fd == nil { + continue + } + validStart := validateLine(fd, lineStart) + if validStart <= 0 { + continue + } + lineStart = validStart + if lineEnd > findings[0].LineStart { + validEnd := validateLine(fd, lineEnd) + if validEnd > 0 { + lineEnd = validEnd + } else { + lineEnd = lineStart + } + } else { + lineEnd = lineStart + } + } + var body strings.Builder body.WriteString(fmt.Sprintf("**acig** found %d issue(s) here:\n\n", len(findings))) for i, f := range findings { @@ -170,12 +205,12 @@ func buildReviewComments(v *verdict.Verdict, prFiles []string) []githubclient.Re } comment := githubclient.ReviewComment{ Path: findings[0].File, - Line: findings[0].LineStart, + Line: lineStart, Body: body.String(), } - if findings[0].LineEnd > findings[0].LineStart { - comment.Line = findings[0].LineEnd - comment.StartLine = findings[0].LineStart + if lineEnd > findings[0].LineStart && lineEnd != lineStart { + comment.Line = lineEnd + comment.StartLine = lineStart } comments = append(comments, comment) } @@ -204,6 +239,20 @@ func groupFindings(findings []verdict.Finding) [][]verdict.Finding { return result } +func validateLine(fd *diff.FileDiff, requestedLine int) int { + if fd == nil || fd.IsDelete { + return 0 + } + addedCount := len(fd.Added) + if addedCount == 0 { + return 0 + } + if requestedLine <= addedCount { + return requestedLine + } + return addedCount +} + 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 index 8acc8c6..1f83247 100644 --- a/internal/reporters/github_report_test.go +++ b/internal/reporters/github_report_test.go @@ -6,6 +6,7 @@ import ( "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" @@ -18,6 +19,7 @@ type mockGitHubClient struct { listReviewsErr error deleteReviewCommentsErr error dismissReviewErr error + getPRFileDiffsErr error } type createReviewCall struct { @@ -43,6 +45,15 @@ func (m *mockGitHubClient) ListPRFiles(ctx context.Context, owner, repo string, 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", 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 { diff --git a/internal/reporters/github_test.go b/internal/reporters/github_test.go index 369493c..a8caa87 100644 --- a/internal/reporters/github_test.go +++ b/internal/reporters/github_test.go @@ -61,7 +61,7 @@ func TestBuildReviewComments_FiltersNonPRFiles(t *testing.T) { } prFiles := []string{"a.txt", "b.txt", "c.txt"} - comments := buildReviewComments(v, prFiles) + comments := buildReviewComments(v, prFiles, nil) require.Len(t, comments, 2) require.Equal(t, "a.txt", comments[0].Path) @@ -76,7 +76,7 @@ func TestBuildReviewComments_AllFilesIfPRFilesNil(t *testing.T) { }, } - comments := buildReviewComments(v, nil) + comments := buildReviewComments(v, nil, nil) require.Len(t, comments, 2) } @@ -88,7 +88,7 @@ func TestBuildReviewComments_EmptyPRFiles(t *testing.T) { }, } - comments := buildReviewComments(v, []string{}) + comments := buildReviewComments(v, []string{}, nil) require.Len(t, comments, 1) } @@ -100,7 +100,7 @@ func TestBuildReviewComments_MultiLineComment(t *testing.T) { }, } - comments := buildReviewComments(v, []string{"a.txt"}) + comments := buildReviewComments(v, []string{"a.txt"}, nil) require.Len(t, comments, 1) require.Equal(t, 20, comments[0].Line) @@ -114,7 +114,7 @@ func TestBuildReviewComments_SingleLineComment(t *testing.T) { }, } - comments := buildReviewComments(v, []string{"a.txt"}) + comments := buildReviewComments(v, []string{"a.txt"}, nil) require.Len(t, comments, 1) require.Equal(t, 10, comments[0].Line) @@ -131,7 +131,7 @@ func TestBuildReviewComments_GroupsFindingsByFileAndLine(t *testing.T) { }, } - comments := buildReviewComments(v, []string{"a.txt", "b.txt"}) + comments := buildReviewComments(v, []string{"a.txt", "b.txt"}, nil) require.Len(t, comments, 3) require.Contains(t, comments[0].Body, "Issue 1") From cb6f9f4ed0d91d40639dbbfe13139950f01290c8 Mon Sep 17 00:00:00 2001 From: Lewis Barclay Date: Fri, 8 May 2026 00:29:40 +0100 Subject: [PATCH 13/21] fix: ignore review when empty --- .gitignore | 4 +++- internal/reporters/github.go | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) 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/internal/reporters/github.go b/internal/reporters/github.go index 05733a3..22fab2b 100644 --- a/internal/reporters/github.go +++ b/internal/reporters/github.go @@ -163,7 +163,7 @@ func buildReviewComments(v *verdict.Verdict, prFiles []string, fileDiffs map[str if len(findings) == 0 || findings[0].File == "" || findings[0].LineStart <= 0 { continue } - if len(prFiles) > 0 && !prFilesSet[findings[0].File] { + if len(prFilesSet) > 0 && !prFilesSet[findings[0].File] { continue } From 7a1c11e2e97d46c0e47372d1953ed1425d49058c Mon Sep 17 00:00:00 2001 From: Lewis Barclay Date: Fri, 8 May 2026 01:30:08 +0100 Subject: [PATCH 14/21] fix: resolve 422 Path could not be resolved errors in PR reviews MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three root causes for GitHub API 422 errors when creating review comments: 1. Path normalization: GetPRFileDiffs used f.GetFilename() directly instead of diff.FromFiles() which applies newPath() to strip a/b/ prefixes, causing path mismatches between findings and diff map keys. 2. Line number validation used array indices instead of actual line numbers: validateLine compared requestedLine against len(fd.Added) (array count), but Added stores content strings, not line numbers. A finding at line 50 in a file with 10 added lines would pass validation (50 > 10 → clamped to 10), sending an invalid line to GitHub. Now uses StartLine from diff hunk headers for accurate remapping. 3. Nil fileDiffs bypassed all validation: when GetPRFileDiffs failed (common in CI), fileDiffs was nil and the if fileDiffs != nil block was skipped entirely, allowing unvalidated line numbers and paths through to the API. Now buildReviewComments requires fileDiffs entries for inline comments — missing files are skipped. Key changes: - FileDiff.StartLine tracks actual hunk start line from @@ headers - validateLine uses StartLine + Added count for bounds checking - GetPRFileDiffs uses diff.FromFiles for consistent path normalization - buildReviewComments skips files absent from fileDiffs (fail-safe) - Comprehensive tests for validateLine and buildReviewComments --- internal/diff/diff.go | 13 +- internal/diff/diff_test.go | 20 +++ internal/diff/git.go | 3 + internal/githubclient/review.go | 21 ++- internal/reporters/github.go | 50 ++++---- internal/reporters/github_report_test.go | 2 +- internal/reporters/github_test.go | 155 +++++++++++++++++++++-- 7 files changed, 211 insertions(+), 53 deletions(-) diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 7fccf57..398122a 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -1,12 +1,13 @@ package diff type FileDiff struct { - Path string - Added []string - Removed []string - Patch string - IsNew bool - IsDelete bool + Path string + 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..36a027f 100644 --- a/internal/diff/diff_test.go +++ b/internal/diff/diff_test.go @@ -38,8 +38,10 @@ 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, "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, 2, d.Stats.FilesChanged) } @@ -49,6 +51,24 @@ 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, []string{"\tadded line 1", "\tadded line 2"}, d.Files[0].Added) +} + func TestNewPath(t *testing.T) { tests := []struct { name string diff --git a/internal/diff/git.go b/internal/diff/git.go index 5de9166..5af5b8c 100644 --- a/internal/diff/git.go +++ b/internal/diff/git.go @@ -83,6 +83,9 @@ func Parse(patch string) (*Diff, error) { } for _, h := range f.Hunks { + 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 a394819..3defb00 100644 --- a/internal/githubclient/review.go +++ b/internal/githubclient/review.go @@ -122,19 +122,18 @@ func (c *Client) GetPRFileDiffs(ctx context.Context, owner, repo string, prNumbe result := make(map[string]*diff.FileDiff) for _, f := range files { - fd := &diff.FileDiff{ - Path: f.GetFilename(), - IsNew: f.GetStatus() == "added", - IsDelete: f.GetStatus() == "removed", + if f.Patch == nil || *f.Patch == "" { + continue } - if f.Patch != nil { - d, parseErr := diff.FromFiles(*f.Patch) - if parseErr == nil && len(d.Files) > 0 { - fd.Added = d.Files[0].Added - fd.Removed = d.Files[0].Removed - } + d, parseErr := diff.FromFiles(*f.Patch) + if parseErr != nil || len(d.Files) == 0 { + continue + } + fd := &d.Files[0] + if fd.IsDelete { + continue } - result[f.GetFilename()] = fd + result[fd.Path] = fd } return result, nil } \ No newline at end of file diff --git a/internal/reporters/github.go b/internal/reporters/github.go index 22fab2b..398e326 100644 --- a/internal/reporters/github.go +++ b/internal/reporters/github.go @@ -167,28 +167,21 @@ func buildReviewComments(v *verdict.Verdict, prFiles []string, fileDiffs map[str continue } - lineStart := findings[0].LineStart - lineEnd := findings[0].LineEnd + fd := fileDiffs[findings[0].File] + if fd == nil { + continue + } + validStart := validateLine(fd, findings[0].LineStart) + if validStart <= 0 { + continue + } - if fileDiffs != nil { - fd := fileDiffs[findings[0].File] - if fd == nil { - continue - } - validStart := validateLine(fd, lineStart) - if validStart <= 0 { - continue - } - lineStart = validStart - if lineEnd > findings[0].LineStart { - validEnd := validateLine(fd, lineEnd) - if validEnd > 0 { - lineEnd = validEnd - } else { - lineEnd = lineStart - } - } else { - lineEnd = lineStart + lineStart := validStart + lineEnd := lineStart + if findings[0].LineEnd > findings[0].LineStart { + validEnd := validateLine(fd, findings[0].LineEnd) + if validEnd > 0 { + lineEnd = validEnd } } @@ -208,7 +201,7 @@ func buildReviewComments(v *verdict.Verdict, prFiles []string, fileDiffs map[str Line: lineStart, Body: body.String(), } - if lineEnd > findings[0].LineStart && lineEnd != lineStart { + if lineEnd > lineStart { comment.Line = lineEnd comment.StartLine = lineStart } @@ -243,14 +236,17 @@ func validateLine(fd *diff.FileDiff, requestedLine int) int { if fd == nil || fd.IsDelete { return 0 } - addedCount := len(fd.Added) - if addedCount == 0 { + if fd.StartLine <= 0 || len(fd.Added) == 0 { return 0 } - if requestedLine <= addedCount { - return requestedLine + lastAddedLine := fd.StartLine + len(fd.Added) - 1 + if requestedLine > lastAddedLine { + return lastAddedLine + } + if requestedLine < fd.StartLine { + return fd.StartLine } - return addedCount + return requestedLine } func verdictJSON(v *verdict.Verdict) (string, error) { diff --git a/internal/reporters/github_report_test.go b/internal/reporters/github_report_test.go index 1f83247..716e231 100644 --- a/internal/reporters/github_report_test.go +++ b/internal/reporters/github_report_test.go @@ -50,7 +50,7 @@ func (m *mockGitHubClient) GetPRFileDiffs(ctx context.Context, owner, repo strin return nil, m.getPRFileDiffsErr } return map[string]*diff.FileDiff{ - "a.txt": {Path: "a.txt", Added: []string{"line1", "line2", "line3", "line4", "line5", "line6", "line7", "line8", "line9", "line10"}}, + "a.txt": {Path: "a.txt", StartLine: 1, Added: []string{"line1", "line2", "line3", "line4", "line5", "line6", "line7", "line8", "line9", "line10"}}, }, nil } diff --git a/internal/reporters/github_test.go b/internal/reporters/github_test.go index a8caa87..dff9805 100644 --- a/internal/reporters/github_test.go +++ b/internal/reporters/github_test.go @@ -3,6 +3,7 @@ package reporters import ( "testing" + "github.com/helloodokai/acig/internal/diff" "github.com/helloodokai/acig/internal/verdict" "github.com/stretchr/testify/require" ) @@ -61,14 +62,18 @@ func TestBuildReviewComments_FiltersNonPRFiles(t *testing.T) { } prFiles := []string{"a.txt", "b.txt", "c.txt"} - comments := buildReviewComments(v, prFiles, nil) + fileDiffs := map[string]*diff.FileDiff{ + "a.txt": {Path: "a.txt", StartLine: 5, Added: []string{"l5", "l6", "l7", "l8", "l9", "l10"}}, + "b.txt": {Path: "b.txt", StartLine: 25, 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_AllFilesIfPRFilesNil(t *testing.T) { +func TestBuildReviewComments_NoCommentsWhenFileDiffsNil(t *testing.T) { v := &verdict.Verdict{ Findings: []verdict.Finding{ {File: "a.txt", LineStart: 10, Title: "Issue A"}, @@ -78,17 +83,20 @@ func TestBuildReviewComments_AllFilesIfPRFilesNil(t *testing.T) { comments := buildReviewComments(v, nil, nil) - require.Len(t, comments, 2) + require.Len(t, comments, 0) } func TestBuildReviewComments_EmptyPRFiles(t *testing.T) { v := &verdict.Verdict{ Findings: []verdict.Finding{ - {File: "a.txt", LineStart: 10, Title: "Issue A"}, + {File: "a.txt", LineStart: 5, Title: "Issue A"}, }, } - comments := buildReviewComments(v, []string{}, nil) + fileDiffs := map[string]*diff.FileDiff{ + "a.txt": {Path: "a.txt", StartLine: 1, Added: []string{"l1", "l2", "l3", "l4", "l5"}}, + } + comments := buildReviewComments(v, []string{}, fileDiffs) require.Len(t, comments, 1) } @@ -100,7 +108,10 @@ func TestBuildReviewComments_MultiLineComment(t *testing.T) { }, } - comments := buildReviewComments(v, []string{"a.txt"}, nil) + fileDiffs := map[string]*diff.FileDiff{ + "a.txt": {Path: "a.txt", StartLine: 1, Added: make([]string, 30)}, + } + comments := buildReviewComments(v, []string{"a.txt"}, fileDiffs) require.Len(t, comments, 1) require.Equal(t, 20, comments[0].Line) @@ -114,7 +125,10 @@ func TestBuildReviewComments_SingleLineComment(t *testing.T) { }, } - comments := buildReviewComments(v, []string{"a.txt"}, nil) + fileDiffs := map[string]*diff.FileDiff{ + "a.txt": {Path: "a.txt", StartLine: 1, Added: make([]string, 20)}, + } + comments := buildReviewComments(v, []string{"a.txt"}, fileDiffs) require.Len(t, comments, 1) require.Equal(t, 10, comments[0].Line) @@ -131,9 +145,134 @@ func TestBuildReviewComments_GroupsFindingsByFileAndLine(t *testing.T) { }, } - comments := buildReviewComments(v, []string{"a.txt", "b.txt"}, nil) + fileDiffs := map[string]*diff.FileDiff{ + "a.txt": {Path: "a.txt", StartLine: 1, Added: make([]string, 50)}, + "b.txt": {Path: "b.txt", StartLine: 1, 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 range", + fd: &diff.FileDiff{StartLine: 50, Added: []string{"l1", "l2", "l3"}}, + requestedLine: 51, + expected: 51, + }, + { + name: "at start", + fd: &diff.FileDiff{StartLine: 50, Added: []string{"l1", "l2", "l3"}}, + requestedLine: 50, + expected: 50, + }, + { + name: "at end", + fd: &diff.FileDiff{StartLine: 50, Added: []string{"l1", "l2", "l3"}}, + requestedLine: 52, + expected: 52, + }, + { + name: "beyond last line clamps", + fd: &diff.FileDiff{StartLine: 50, Added: []string{"l1", "l2", "l3"}}, + requestedLine: 100, + expected: 52, + }, + { + name: "before start line clamps to start", + fd: &diff.FileDiff{StartLine: 50, Added: []string{"l1", "l2", "l3"}}, + 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, Added: []string{"l1"}}, + requestedLine: 1, + expected: 0, + }, + { + name: "zero start line returns 0", + fd: &diff.FileDiff{StartLine: 0, Added: []string{"l1"}}, + requestedLine: 1, + expected: 0, + }, + { + name: "empty added lines returns 0", + fd: &diff.FileDiff{StartLine: 1, Added: []string{}}, + requestedLine: 1, + expected: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := validateLine(tt.fd, tt.requestedLine) + require.Equal(t, tt.expected, result) + }) + } +} + +func TestBuildReviewComments_LineRemappingWithStartLine(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, 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_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, 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, Added: []string{"l10", "l11", "l12"}}, + } + comments := buildReviewComments(v, []string{"a.txt"}, fileDiffs) + + require.Len(t, comments, 1) + require.Equal(t, 12, comments[0].Line) } \ No newline at end of file From f08cc8eef9c0b48396b4de0a1d43fba91ab7a4f6 Mon Sep 17 00:00:00 2001 From: Lewis Barclay Date: Fri, 8 May 2026 01:31:25 +0100 Subject: [PATCH 15/21] chore: bump version to 1.5.5 --- internal/version/version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/version/version.go b/internal/version/version.go index d869262..07c61a8 100644 --- a/internal/version/version.go +++ b/internal/version/version.go @@ -1,3 +1,3 @@ package version -var Version = "1.5.2" \ No newline at end of file +var Version = "1.5.5" \ No newline at end of file From e18dc93882b0474e94edfbd4d220c58bb8dd7f3e Mon Sep 17 00:00:00 2001 From: Lewis Barclay Date: Fri, 8 May 2026 01:55:31 +0100 Subject: [PATCH 16/21] fix: use hunk ranges for line validation, fix Line could not be resolved The previous fix used StartLine + len(Added) to compute valid line ranges, but Added only contains + lines, not context lines. GitHub's line parameter must reference ANY line visible in the PR diff hunks, including context lines (lines starting with space in the diff). Now tracks HunkRange{Start, End} from each @@ -old,count +new,count @@ hunk header, where Start=NewStartLine and End=NewStartLine+NewLines-1. validateLine checks if the requested line falls within any hunk range, and clamps to the nearest range start if it doesn't. Also adds integration tests using httptest to mock GitHub API responses, validating path normalization, hunk range parsing, and review payload construction. --- internal/diff/diff.go | 20 +- internal/diff/diff_test.go | 25 ++ internal/diff/git.go | 6 + internal/githubclient/review_test.go | 472 +++++++++++++++++++++-- internal/reporters/github.go | 25 +- internal/reporters/github_report_test.go | 2 +- internal/reporters/github_test.go | 123 ++++-- internal/version/version.go | 2 +- 8 files changed, 594 insertions(+), 81 deletions(-) diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 398122a..f37a50a 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -1,13 +1,19 @@ package diff +type HunkRange struct { + Start int + End int +} + type FileDiff struct { - Path string - StartLine int - 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 36a027f..df67d68 100644 --- a/internal/diff/diff_test.go +++ b/internal/diff/diff_test.go @@ -39,9 +39,11 @@ func TestParse(t *testing.T) { 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) } @@ -66,9 +68,32 @@ index abc1234..def5678 100644 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 5af5b8c..c7eecc8 100644 --- a/internal/diff/git.go +++ b/internal/diff/git.go @@ -83,6 +83,12 @@ 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) } diff --git a/internal/githubclient/review_test.go b/internal/githubclient/review_test.go index f9550c0..1a1fd70 100644 --- a/internal/githubclient/review_test.go +++ b/internal/githubclient/review_test.go @@ -1,45 +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 TestReviewCommentStruct(t *testing.T) { - rc := ReviewComment{ - Path: "test.txt", - Line: 10, - StartLine: 5, - Body: "test body", - } - require.Equal(t, "test.txt", rc.Path) - require.Equal(t, 10, rc.Line) - require.Equal(t, 5, rc.StartLine) - require.Equal(t, "test body", rc.Body) +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 TestReviewCommentStruct_NoStartLine(t *testing.T) { - rc := ReviewComment{ - Path: "test.txt", - Line: 10, - Body: "test body", +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, + }, } - require.Equal(t, "test.txt", rc.Path) - require.Equal(t, 10, rc.Line) - require.Equal(t, "test body", rc.Body) - require.Equal(t, 0, rc.StartLine) + + 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 TestReviewCommentStruct_MultiLine(t *testing.T) { - rc := ReviewComment{ - Path: "test.txt", - Line: 20, - StartLine: 10, - Body: "multi-line comment", - } - require.Equal(t, "test.txt", rc.Path) - require.Equal(t, "multi-line comment", rc.Body) - require.True(t, rc.StartLine > 0) - require.Greater(t, rc.Line, rc.StartLine) +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/reporters/github.go b/internal/reporters/github.go index 398e326..4cf2007 100644 --- a/internal/reporters/github.go +++ b/internal/reporters/github.go @@ -236,17 +236,28 @@ func validateLine(fd *diff.FileDiff, requestedLine int) int { if fd == nil || fd.IsDelete { return 0 } - if fd.StartLine <= 0 || len(fd.Added) == 0 { + if len(fd.HunkRanges) == 0 { return 0 } - lastAddedLine := fd.StartLine + len(fd.Added) - 1 - if requestedLine > lastAddedLine { - return lastAddedLine + 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 + } } - if requestedLine < fd.StartLine { - return fd.StartLine + return nearest.Start +} + +func abs(x int) int { + if x < 0 { + return -x } - return requestedLine + return x } func verdictJSON(v *verdict.Verdict) (string, error) { diff --git a/internal/reporters/github_report_test.go b/internal/reporters/github_report_test.go index 716e231..05abb5c 100644 --- a/internal/reporters/github_report_test.go +++ b/internal/reporters/github_report_test.go @@ -50,7 +50,7 @@ func (m *mockGitHubClient) GetPRFileDiffs(ctx context.Context, owner, repo strin return nil, m.getPRFileDiffsErr } return map[string]*diff.FileDiff{ - "a.txt": {Path: "a.txt", StartLine: 1, Added: []string{"line1", "line2", "line3", "line4", "line5", "line6", "line7", "line8", "line9", "line10"}}, + "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 } diff --git a/internal/reporters/github_test.go b/internal/reporters/github_test.go index dff9805..a97301e 100644 --- a/internal/reporters/github_test.go +++ b/internal/reporters/github_test.go @@ -52,6 +52,11 @@ func TestGroupFindings_ZeroLineStart(t *testing.T) { require.Equal(t, "b.txt", groups[0][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{ @@ -63,8 +68,8 @@ func TestBuildReviewComments_FiltersNonPRFiles(t *testing.T) { prFiles := []string{"a.txt", "b.txt", "c.txt"} fileDiffs := map[string]*diff.FileDiff{ - "a.txt": {Path: "a.txt", StartLine: 5, Added: []string{"l5", "l6", "l7", "l8", "l9", "l10"}}, - "b.txt": {Path: "b.txt", StartLine: 25, Added: []string{"l25", "l26", "l27", "l28", "l29", "l30"}}, + "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) @@ -94,7 +99,7 @@ func TestBuildReviewComments_EmptyPRFiles(t *testing.T) { } fileDiffs := map[string]*diff.FileDiff{ - "a.txt": {Path: "a.txt", StartLine: 1, Added: []string{"l1", "l2", "l3", "l4", "l5"}}, + "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) @@ -109,7 +114,7 @@ func TestBuildReviewComments_MultiLineComment(t *testing.T) { } fileDiffs := map[string]*diff.FileDiff{ - "a.txt": {Path: "a.txt", StartLine: 1, Added: make([]string, 30)}, + "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) @@ -126,7 +131,7 @@ func TestBuildReviewComments_SingleLineComment(t *testing.T) { } fileDiffs := map[string]*diff.FileDiff{ - "a.txt": {Path: "a.txt", StartLine: 1, Added: make([]string, 20)}, + "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) @@ -146,8 +151,8 @@ func TestBuildReviewComments_GroupsFindingsByFileAndLine(t *testing.T) { } fileDiffs := map[string]*diff.FileDiff{ - "a.txt": {Path: "a.txt", StartLine: 1, Added: make([]string, 50)}, - "b.txt": {Path: "b.txt", StartLine: 1, Added: make([]string, 30)}, + "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) @@ -164,32 +169,38 @@ func TestValidateLine(t *testing.T) { expected int }{ { - name: "within range", - fd: &diff.FileDiff{StartLine: 50, Added: []string{"l1", "l2", "l3"}}, + 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 start", - fd: &diff.FileDiff{StartLine: 50, Added: []string{"l1", "l2", "l3"}}, + 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: "at end", - fd: &diff.FileDiff{StartLine: 50, Added: []string{"l1", "l2", "l3"}}, - requestedLine: 52, - expected: 52, + 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: "beyond last line clamps", - fd: &diff.FileDiff{StartLine: 50, Added: []string{"l1", "l2", "l3"}}, + 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: 52, + expected: 50, }, { - name: "before start line clamps to start", - fd: &diff.FileDiff{StartLine: 50, Added: []string{"l1", "l2", "l3"}}, + 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, }, @@ -201,33 +212,44 @@ func TestValidateLine(t *testing.T) { }, { name: "deleted file returns 0", - fd: &diff.FileDiff{StartLine: 1, IsDelete: true, Added: []string{"l1"}}, + fd: &diff.FileDiff{StartLine: 1, IsDelete: true, HunkRanges: hr(diff.HunkRange{Start: 1, End: 10}), Added: []string{"l1"}}, requestedLine: 1, expected: 0, }, { - name: "zero start line returns 0", - fd: &diff.FileDiff{StartLine: 0, Added: []string{"l1"}}, + name: "empty hunk ranges returns 0", + fd: &diff.FileDiff{StartLine: 1, HunkRanges: nil, Added: []string{"l1"}}, requestedLine: 1, expected: 0, }, { - name: "empty added lines returns 0", - fd: &diff.FileDiff{StartLine: 1, Added: []string{}}, - 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) { - result := validateLine(tt.fd, tt.requestedLine) - require.Equal(t, tt.expected, result) + 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_LineRemappingWithStartLine(t *testing.T) { +func TestBuildReviewComments_LineInHunkRange(t *testing.T) { v := &verdict.Verdict{ Findings: []verdict.Finding{ {File: "app.go", LineStart: 52, LineEnd: 52, Title: "Issue at line 52"}, @@ -235,7 +257,7 @@ func TestBuildReviewComments_LineRemappingWithStartLine(t *testing.T) { } fileDiffs := map[string]*diff.FileDiff{ - "app.go": {Path: "app.go", StartLine: 50, Added: []string{"l50", "l51", "l52", "l53", "l54"}}, + "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) @@ -244,6 +266,22 @@ func TestBuildReviewComments_LineRemappingWithStartLine(t *testing.T) { 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{ @@ -253,7 +291,7 @@ func TestBuildReviewComments_SkipsFileNotInFileDiffs(t *testing.T) { } fileDiffs := map[string]*diff.FileDiff{ - "present.go": {Path: "present.go", StartLine: 1, Added: []string{"l1", "l2", "l3", "l4", "l5", "l6", "l7", "l8", "l9", "l10"}}, + "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) @@ -269,10 +307,27 @@ func TestBuildReviewComments_ClampsOutOfRangeLine(t *testing.T) { } fileDiffs := map[string]*diff.FileDiff{ - "a.txt": {Path: "a.txt", StartLine: 10, Added: []string{"l10", "l11", "l12"}}, + "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, 12, comments[0].Line) + 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) } \ No newline at end of file diff --git a/internal/version/version.go b/internal/version/version.go index 07c61a8..630bfa1 100644 --- a/internal/version/version.go +++ b/internal/version/version.go @@ -1,3 +1,3 @@ package version -var Version = "1.5.5" \ No newline at end of file +var Version = "1.5.6" \ No newline at end of file From 972087a3b7add49e9afb1c4f44388dde75974fda Mon Sep 17 00:00:00 2001 From: Lewis Barclay Date: Fri, 8 May 2026 02:03:57 +0100 Subject: [PATCH 17/21] fix: add auth headers and debug output to acig install script The checksums.txt download was failing silently in CI, returning an empty expected hash. Added GitHub API auth headers for rate limiting and a check that the expected hash is non-empty with file contents output for debugging. --- .github/workflows/acig.yml | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/.github/workflows/acig.yml b/.github/workflows/acig.yml index 8842bdf..6036a78 100644 --- a/.github/workflows/acig.yml +++ b/.github/workflows/acig.yml @@ -28,7 +28,7 @@ jobs: env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | - ACIG_VERSION=$(curl -sL "https://api.github.com/repos/helloodokai/acig/releases/latest" | grep '"tag_name"' | sed 's/.*v\([0-9.]*\).*/\1/') + ACIG_VERSION=$(curl -sL -H "Accept: application/vnd.github+json" -H "Authorization: Bearer $GITHUB_TOKEN" "https://api.github.com/repos/helloodokai/acig/releases/latest" | grep '"tag_name"' | sed 's/.*v\([0-9.]*\).*/\1/') if [ -z "$ACIG_VERSION" ]; then echo "::error::could not determine latest acig version" exit 1 @@ -40,7 +40,13 @@ jobs: curl -sL "${RELEASE_URL}/acig_linux_amd64.tar.gz" -o /tmp/acig_linux_amd64.tar.gz 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" + echo "checksums.txt contents:" + 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" From ecf5991806b8d431a6025159b512c64c59fca3c4 Mon Sep 17 00:00:00 2001 From: Lewis Barclay Date: Fri, 8 May 2026 02:15:41 +0100 Subject: [PATCH 18/21] fix: use gh CLI for authenticated release downloads Replaces raw curl calls to GitHub API with `gh release` commands, which handle authentication via GITHUB_TOKEN automatically. This eliminates SSRF risk from user-controlled URL construction, command injection from untrusted version interpolation, and insecure credential handling in curl headers. --- .github/workflows/acig.yml | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/.github/workflows/acig.yml b/.github/workflows/acig.yml index 6036a78..8f90c5c 100644 --- a/.github/workflows/acig.yml +++ b/.github/workflows/acig.yml @@ -28,22 +28,19 @@ jobs: env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | - ACIG_VERSION=$(curl -sL -H "Accept: application/vnd.github+json" -H "Authorization: Bearer $GITHUB_TOKEN" "https://api.github.com/repos/helloodokai/acig/releases/latest" | grep '"tag_name"' | sed 's/.*v\([0-9.]*\).*/\1/') + 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}" - RELEASE_URL="https://github.com/helloodokai/acig/releases/download/v${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}') if [ -z "$EXPECTED" ]; then echo "::error::checksums.txt does not contain acig_linux_amd64.tar.gz" - echo "checksums.txt contents:" cat checksums.txt exit 1 fi From a3c02aa56ae4af2faf5880003b3b94a04ef60290 Mon Sep 17 00:00:00 2001 From: Lewis Barclay Date: Fri, 8 May 2026 11:11:27 +0100 Subject: [PATCH 19/21] fix: filter hallucinated file paths from critic findings Critics like perf_smell and test_coverage_smell (gpt-oss models) would hallucinate file paths not present in the diff (e.g. inventing service/get_plan_context.go in a TypeScript codebase). These fabricated findings caused GitHub review comment 422 errors and polluted the verdict table with references to non-existent files. Three-layer defense: - Pipeline: FilterHallucinatedPaths() strips findings referencing files not in the diff before verdict finalization, with warning logs - Reporter: Defensive slog.Warn on skip points in buildReviewComments - Prompts: Added CRITICAL constraint + explicit file list ({{.ChangedFiles}}) to all 6 critic prompts to reduce model hallucination at the source Bump version to 1.6.0 --- internal/critics/helpers.go | 11 ++ internal/critics/prompts/adjudicator.md | 2 + internal/critics/prompts/perf_smell.md | 2 + internal/critics/prompts/risk_classifier.md | 2 + internal/critics/prompts/security_smell.md | 2 + internal/critics/prompts/style_conformance.md | 2 + .../critics/prompts/test_coverage_smell.md | 2 + internal/pipeline/pipeline.go | 10 +- internal/pipeline/pipeline_test.go | 50 +++++++++ internal/reporters/github.go | 2 + internal/verdict/validate_paths.go | 37 +++++++ internal/verdict/validate_paths_test.go | 100 ++++++++++++++++++ internal/version/version.go | 2 +- 13 files changed, 221 insertions(+), 3 deletions(-) create mode 100644 internal/verdict/validate_paths.go create mode 100644 internal/verdict/validate_paths_test.go 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/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 7588f89..39bfbb5 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,8 +203,9 @@ 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.FilterHallucinatedPaths(v.Findings, diffPaths) v.Findings = verdict.FilterFindings(v.Findings, suppressions) v.TotalCostUSD = ledger.Spent() v.BudgetRemainingUSD = ledger.Remaining() diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 8950800..339f95a 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,54 @@ func TestComputeRisk(t *testing.T) { } } +func TestFinalize_FilterHallucinatedPaths(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: "service/get_plan_context.go", LineStart: 5, Severity: verdict.SeverityHigh, Critic: "perf_smell", Title: "hallucinated Go file"}, + {File: "src/services/planContextService.js", LineStart: 20, Severity: verdict.SeverityLow, Critic: "test_coverage_smell", Title: "hallucinated JS 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) +} + +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 4cf2007..4a166d3 100644 --- a/internal/reporters/github.go +++ b/internal/reporters/github.go @@ -164,11 +164,13 @@ func buildReviewComments(v *verdict.Verdict, prFiles []string, fileDiffs map[str 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 } validStart := validateLine(fd, findings[0].LineStart) diff --git a/internal/verdict/validate_paths.go b/internal/verdict/validate_paths.go new file mode 100644 index 0000000..9a063e1 --- /dev/null +++ b/internal/verdict/validate_paths.go @@ -0,0 +1,37 @@ +package verdict + +import "log/slog" + +// FilterHallucinatedPaths removes findings whose File field does not appear in +// the set of known diff paths. Findings with an empty File or a File that is +// present in diffPaths are kept. Findings referencing unknown files are logged +// as warnings and dropped. +func FilterHallucinatedPaths(findings []Finding, diffPaths []string) []Finding { + if len(diffPaths) == 0 { + return findings + } + + valid := make(map[string]bool, len(diffPaths)) + for _, p := range diffPaths { + valid[p] = true + } + + var kept []Finding + for _, f := range findings { + if f.File == "" { + kept = append(kept, f) + continue + } + if valid[f.File] { + kept = append(kept, f) + continue + } + slog.Warn("filtering hallucinated file path from finding", + "critic", f.Critic, + "file", f.File, + "line_start", f.LineStart, + "title", f.Title, + ) + } + return kept +} \ 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..0513862 --- /dev/null +++ b/internal/verdict/validate_paths_test.go @@ -0,0 +1,100 @@ +package verdict + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestFilterHallucinatedPaths_KeepsValidFindings(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"} + + result := FilterHallucinatedPaths(findings, diffPaths) + require.Len(t, result, 2) + require.Equal(t, "apps/backend/src/lib/tools/plan-tools.ts", result[0].File) + require.Equal(t, "src/main.ts", result[1].File) +} + +func TestFilterHallucinatedPaths_RemovesHallucinatedPaths(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"} + + result := FilterHallucinatedPaths(findings, diffPaths) + require.Len(t, result, 1) + require.Equal(t, "apps/backend/src/lib/tools/plan-tools.ts", result[0].File) +} + +func TestFilterHallucinatedPaths_KeepsEmptyFileFindings(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"} + + result := FilterHallucinatedPaths(findings, diffPaths) + require.Len(t, result, 2) +} + +func TestFilterHallucinatedPaths_EmptyDiffPathsKeepsAll(t *testing.T) { + findings := []Finding{ + {File: "madeup.go", LineStart: 10, Title: "hallucinated"}, + {File: "also_madeup.js", LineStart: 20, Title: "also hallucinated"}, + } + + result := FilterHallucinatedPaths(findings, nil) + require.Len(t, result, 2) + + result = FilterHallucinatedPaths(findings, []string{}) + require.Len(t, result, 2) +} + +func TestFilterHallucinatedPaths_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"} + + result := FilterHallucinatedPaths(findings, diffPaths) + require.Len(t, result, 0) +} + +func TestFilterHallucinatedPaths_NilFindings(t *testing.T) { + result := FilterHallucinatedPaths(nil, []string{"a.txt"}) + require.Len(t, result, 0) +} + +func TestFilterHallucinatedPaths_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"} + + result := FilterHallucinatedPaths(findings, diffPaths) + require.Len(t, result, 1) + require.Equal(t, "risk_classifier", result[0].Critic) + require.Equal(t, SeverityHigh, result[0].Severity) + require.Equal(t, "SQL injection", result[0].Title) + require.Equal(t, "src/db.ts", result[0].File) + require.Equal(t, 42, result[0].LineStart) + require.Equal(t, 45, result[0].LineEnd) + require.Equal(t, "use parameterized query", result[0].SuggestedFix) +} \ No newline at end of file diff --git a/internal/version/version.go b/internal/version/version.go index 630bfa1..e0b9c15 100644 --- a/internal/version/version.go +++ b/internal/version/version.go @@ -1,3 +1,3 @@ package version -var Version = "1.5.6" \ No newline at end of file +var Version = "1.6.0" \ No newline at end of file From cf8aaaa01746c93bb522782f13243ffabc5781e4 Mon Sep 17 00:00:00 2001 From: Lewis Barclay Date: Fri, 8 May 2026 13:30:07 +0100 Subject: [PATCH 20/21] fix: categorize findings into in-diff, dangling, and hallucinated - Replace FilterHallucinatedPaths with CategorizeFindingsByPath that splits findings into: in-diff (can be inline-commented), dangling (plausible suggestions like test files not yet in the PR), and hallucinated (fabricated paths with no relation to the diff) - Dangling findings are kept in Verdict.DanglingFindings and shown in the review body under 'Observations' but NOT posted as inline review comments (which would cause 422 errors) - Test file suggestions (e.g. plan-tools.test.ts) are now correctly recognized as plausible instead of being discarded - Upgrade GitHub review body with emoji severity badges, separated sections for inline vs observation findings, collapsible critic results table, and cleaner formatting - Upgrade markdown reporter with same improved formatting Bump version to 1.7.0 --- internal/pipeline/pipeline.go | 12 +- internal/pipeline/pipeline_test.go | 6 +- internal/reporters/github.go | 83 ++++++++- internal/reporters/markdown.go | 96 ++++++++--- internal/verdict/validate_paths.go | 213 ++++++++++++++++++++++-- internal/verdict/validate_paths_test.go | 151 +++++++++++++---- internal/verdict/verdict.go | 1 + internal/version/version.go | 2 +- 8 files changed, 481 insertions(+), 83 deletions(-) diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 39bfbb5..a79f53b 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -205,8 +205,16 @@ func hasConflict(results []verdict.CriticResult) bool { func finalize(v *verdict.Verdict, ledger *budget.Ledger, suppressions []verdict.Suppression, diffPaths []string) { v.Findings = verdict.DedupeFindings(v.Findings) - v.Findings = verdict.FilterHallucinatedPaths(v.Findings, diffPaths) - 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 339f95a..3c6a4d2 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -85,12 +85,12 @@ func TestComputeRisk(t *testing.T) { } } -func TestFinalize_FilterHallucinatedPaths(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"}, - {File: "src/services/planContextService.js", LineStart: 20, Severity: verdict.SeverityLow, Critic: "test_coverage_smell", Title: "hallucinated JS file"}, }, } ledger, _ := budget.NewLedger(1.0) @@ -100,6 +100,8 @@ func TestFinalize_FilterHallucinatedPaths(t *testing.T) { 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) { diff --git a/internal/reporters/github.go b/internal/reporters/github.go index 4a166d3..aa1109b 100644 --- a/internal/reporters/github.go +++ b/internal/reporters/github.go @@ -129,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) @@ -150,6 +193,30 @@ func (r *GitHubReporter) buildReviewBody(v *verdict.Verdict) string { return body.String() } +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 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 index 9a063e1..8fa76dc 100644 --- a/internal/verdict/validate_paths.go +++ b/internal/verdict/validate_paths.go @@ -2,13 +2,20 @@ package verdict import "log/slog" -// FilterHallucinatedPaths removes findings whose File field does not appear in -// the set of known diff paths. Findings with an empty File or a File that is -// present in diffPaths are kept. Findings referencing unknown files are logged -// as warnings and dropped. -func FilterHallucinatedPaths(findings []Finding, diffPaths []string) []Finding { +// 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 + return findings, nil, nil } valid := make(map[string]bool, len(diffPaths)) @@ -16,22 +23,208 @@ func FilterHallucinatedPaths(findings []Finding, diffPaths []string) []Finding { valid[p] = true } - var kept []Finding for _, f := range findings { if f.File == "" { - kept = append(kept, f) + inDiff = append(inDiff, f) continue } if valid[f.File] { - kept = append(kept, f) + 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 kept + 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 index 0513862..d23a3ba 100644 --- a/internal/verdict/validate_paths_test.go +++ b/internal/verdict/validate_paths_test.go @@ -6,20 +6,20 @@ import ( "github.com/stretchr/testify/require" ) -func TestFilterHallucinatedPaths_KeepsValidFindings(t *testing.T) { +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"} - result := FilterHallucinatedPaths(findings, diffPaths) - require.Len(t, result, 2) - require.Equal(t, "apps/backend/src/lib/tools/plan-tools.ts", result[0].File) - require.Equal(t, "src/main.ts", result[1].File) + inDiff, dangling, hallucinated := CategorizeFindingsByPath(findings, diffPaths) + require.Len(t, inDiff, 2) + require.Empty(t, dangling) + require.Empty(t, hallucinated) } -func TestFilterHallucinatedPaths_RemovesHallucinatedPaths(t *testing.T) { +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"}, @@ -27,36 +27,70 @@ func TestFilterHallucinatedPaths_RemovesHallucinatedPaths(t *testing.T) { } diffPaths := []string{"apps/backend/src/lib/tools/plan-tools.ts", ".charters/ch-2026-05-07-12f7a9.spec.md"} - result := FilterHallucinatedPaths(findings, diffPaths) - require.Len(t, result, 1) - require.Equal(t, "apps/backend/src/lib/tools/plan-tools.ts", result[0].File) + 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 TestFilterHallucinatedPaths_KeepsEmptyFileFindings(t *testing.T) { +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"} - result := FilterHallucinatedPaths(findings, diffPaths) - require.Len(t, result, 2) + inDiff, dangling, hallucinated := CategorizeFindingsByPath(findings, diffPaths) + require.Len(t, inDiff, 2) + require.Empty(t, dangling) + require.Empty(t, hallucinated) } -func TestFilterHallucinatedPaths_EmptyDiffPathsKeepsAll(t *testing.T) { +func TestCategorizeFindingsByPath_EmptyDiffPathsKeepsAll(t *testing.T) { findings := []Finding{ {File: "madeup.go", LineStart: 10, Title: "hallucinated"}, {File: "also_madeup.js", LineStart: 20, Title: "also hallucinated"}, } - result := FilterHallucinatedPaths(findings, nil) - require.Len(t, result, 2) + inDiff, _, _ := CategorizeFindingsByPath(findings, nil) + require.Len(t, inDiff, 2) - result = FilterHallucinatedPaths(findings, []string{}) - require.Len(t, result, 2) + inDiff, dangling, hallucinated := CategorizeFindingsByPath(findings, []string{}) + require.Len(t, inDiff, 2) + require.Empty(t, dangling) + require.Empty(t, hallucinated) } -func TestFilterHallucinatedPaths_AllHallucinated(t *testing.T) { +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"}, @@ -64,16 +98,20 @@ func TestFilterHallucinatedPaths_AllHallucinated(t *testing.T) { } diffPaths := []string{".charters/ch-2026-05-07-12f7a9.spec.md"} - result := FilterHallucinatedPaths(findings, diffPaths) - require.Len(t, result, 0) + inDiff, dangling, hallucinated := CategorizeFindingsByPath(findings, diffPaths) + require.Empty(t, inDiff) + require.Empty(t, dangling) + require.Len(t, hallucinated, 3) } -func TestFilterHallucinatedPaths_NilFindings(t *testing.T) { - result := FilterHallucinatedPaths(nil, []string{"a.txt"}) - require.Len(t, result, 0) +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 TestFilterHallucinatedPaths_PreservesFindingFields(t *testing.T) { +func TestCategorizeFindingsByPath_PreservesFindingFields(t *testing.T) { findings := []Finding{ { Critic: "risk_classifier", @@ -88,13 +126,60 @@ func TestFilterHallucinatedPaths_PreservesFindingFields(t *testing.T) { } diffPaths := []string{"src/db.ts"} - result := FilterHallucinatedPaths(findings, diffPaths) - require.Len(t, result, 1) - require.Equal(t, "risk_classifier", result[0].Critic) - require.Equal(t, SeverityHigh, result[0].Severity) - require.Equal(t, "SQL injection", result[0].Title) - require.Equal(t, "src/db.ts", result[0].File) - require.Equal(t, 42, result[0].LineStart) - require.Equal(t, 45, result[0].LineEnd) - require.Equal(t, "use parameterized query", result[0].SuggestedFix) + 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 e0b9c15..1096745 100644 --- a/internal/version/version.go +++ b/internal/version/version.go @@ -1,3 +1,3 @@ package version -var Version = "1.6.0" \ No newline at end of file +var Version = "1.7.0" \ No newline at end of file From d0344f2084ef5053c49188f1b08be106f8b880bf Mon Sep 17 00:00:00 2001 From: Lewis Barclay Date: Fri, 8 May 2026 14:22:31 +0100 Subject: [PATCH 21/21] fix: post inline review comments for findings with no line number Findings with a file but LineStart=0 (common from risk_classifier and test_coverage_smell) were being silently dropped because groupFindings and buildReviewComments both skipped them. This caused reviews to have no inline comments at all. Now: findings with LineStart=0 are grouped by file and clamped to the first hunk range of the diff, so they still appear as inline comments on the correct file in the PR. Bump version to 1.8.0 --- internal/reporters/github.go | 37 +++++++++++++++++++++---------- internal/reporters/github_test.go | 26 ++++++++++++++++++++-- internal/version/version.go | 2 +- 3 files changed, 50 insertions(+), 15 deletions(-) diff --git a/internal/reporters/github.go b/internal/reporters/github.go index aa1109b..8efc876 100644 --- a/internal/reporters/github.go +++ b/internal/reporters/github.go @@ -227,7 +227,7 @@ func buildReviewComments(v *verdict.Verdict, prFiles []string, fileDiffs map[str } 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] { @@ -240,17 +240,26 @@ func buildReviewComments(v *verdict.Verdict, prFiles []string, fileDiffs map[str slog.Warn("skipping finding with no diff data", "file", findings[0].File, "critic", findings[0].Critic, "title", findings[0].Title) continue } - validStart := validateLine(fd, findings[0].LineStart) + + 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 } - lineStart := validStart - lineEnd := lineStart - if findings[0].LineEnd > findings[0].LineStart { + commentLineStart := validStart + commentLineEnd := commentLineStart + if findings[0].LineEnd > lineStart { validEnd := validateLine(fd, findings[0].LineEnd) if validEnd > 0 { - lineEnd = validEnd + commentLineEnd = validEnd } } @@ -267,12 +276,12 @@ func buildReviewComments(v *verdict.Verdict, prFiles []string, fileDiffs map[str } comment := githubclient.ReviewComment{ Path: findings[0].File, - Line: lineStart, + Line: commentLineStart, Body: body.String(), } - if lineEnd > lineStart { - comment.Line = lineEnd - comment.StartLine = lineStart + if commentLineEnd > commentLineStart { + comment.Line = commentLineEnd + comment.StartLine = commentLineStart } comments = append(comments, comment) } @@ -284,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) } diff --git a/internal/reporters/github_test.go b/internal/reporters/github_test.go index a97301e..35239f7 100644 --- a/internal/reporters/github_test.go +++ b/internal/reporters/github_test.go @@ -44,12 +44,15 @@ func TestGroupFindings_EmptyFile(t *testing.T) { 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, 1) - require.Equal(t, "b.txt", groups[0][0].File) + 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 @@ -330,4 +333,23 @@ func TestBuildReviewComments_LineInSecondHunkRange(t *testing.T) { 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/version/version.go b/internal/version/version.go index 1096745..e3063ff 100644 --- a/internal/version/version.go +++ b/internal/version/version.go @@ -1,3 +1,3 @@ package version -var Version = "1.7.0" \ No newline at end of file +var Version = "1.8.0" \ No newline at end of file