From 4ea80cad1f9a7bc52af21922c7b322656140fc5b Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Sun, 28 Jun 2026 21:59:27 -0400 Subject: [PATCH] fix: address swarm review of the inline PR review (PR #18) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From PR #18's own review — the FIRST run with consensus consolidation live (it posted one ranked consensus comment across 7 models instead of 7 walls). - Anchor inline comments to ADDED lines only (parseDiffNewLines no longer records context lines): Gitea reliably accepts comments on added lines, so the all-or-nothing review POST won't be rejected for an off-change anchor. - Span-scan anchoring (anchorLine): a cluster whose min line is just outside the diff still anchors if any line in its [line,maxLine] span is an added line. - Path normalization (normPath) on both diff and finding paths, so "./pkg/x.go" vs "pkg/x.go" vs a "b/"-prefixed diff path all match (and the comment Path is the normalized repo-relative one). - deletePriorReviews: paginate (cap 10 pages) and bail on a non-2xx GET, so a stale marked review past page 1 is still removed and the "replace not stack" guarantee holds better under failure. - mergeIntoCluster backfills an empty title/detail from any report (not only a strictly-higher-severity one). - Rename inlineReviewHTTPTime -> inlineReviewHTTPTimeout (emit.go convention). Graded the run's 36 findings in gadfly-reports (notably a 7/7-consensus deletePriorReviews error-handling finding — the consensus signal working as intended). XSS-framed findings marked false (Gitea sanitizes review markdown); trusted-input and gofmt-clean ones likewise. Tests updated for added-only anchoring + span scan. gofmt/vet/bash -n clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd/gadfly/consensus.go | 9 +++ cmd/gadfly/review.go | 126 +++++++++++++++++++++++++------------- cmd/gadfly/review_test.go | 55 +++++++++++------ 3 files changed, 131 insertions(+), 59 deletions(-) diff --git a/cmd/gadfly/consensus.go b/cmd/gadfly/consensus.go index cb0bca7..fe6beb5 100644 --- a/cmd/gadfly/consensus.go +++ b/cmd/gadfly/consensus.go @@ -382,6 +382,15 @@ func mergeIntoCluster(c *cluster, f outFinding, model string) { if f.Line > c.maxLine { c.maxLine = f.Line } + // Backfill an empty title/detail from any report, regardless of severity, so a + // higher-severity-but-terse finding doesn't leave the cluster without context. + if strings.TrimSpace(c.title) == "" && strings.TrimSpace(f.Title) != "" { + c.title = f.Title + } + if strings.TrimSpace(c.detail) == "" && strings.TrimSpace(f.Detail) != "" { + c.detail = f.Detail + } + // A strictly-higher-severity report takes over the title/detail. if sevRank(f.Severity) > sevRank(c.severity) { c.severity = f.Severity if strings.TrimSpace(f.Title) != "" { diff --git a/cmd/gadfly/review.go b/cmd/gadfly/review.go index 588ecfa..fbeaf99 100644 --- a/cmd/gadfly/review.go +++ b/cmd/gadfly/review.go @@ -33,8 +33,8 @@ const ( // maxInlineComments caps how many inline comments one review carries, so a // huge diff can't produce a wall of annotations. Clusters are pre-sorted by // agreement×severity, so the cap keeps the most important. - maxInlineComments = 25 - inlineReviewHTTPTime = 20 * time.Second + maxInlineComments = 25 + inlineReviewHTTPTimeout = 20 * time.Second ) // reviewComment is one inline comment in a Gitea pull review. Field names match @@ -76,7 +76,7 @@ func postInlineReview(clusters []cluster) { return // nothing anchors to a changed line; the consensus comment covers it } - client := &http.Client{Timeout: inlineReviewHTTPTime} + client := &http.Client{Timeout: inlineReviewHTTPTimeout} base := fmt.Sprintf("%s/pulls/%s/reviews", api, pr) deletePriorReviews(client, base, token) // avoid stacking on re-runs @@ -92,11 +92,12 @@ func postInlineReview(clusters []cluster) { func inlineComments(clusters []cluster, addable map[string]map[int]bool) []reviewComment { var out []reviewComment for _, c := range clusters { - lines := addable[c.file] - if lines == nil || c.line <= 0 || !lines[c.line] { + path := normPath(c.file) + anchor := anchorLine(addable[path], c.line, c.maxLine) + if anchor == 0 { continue } - out = append(out, reviewComment{Path: c.file, NewPosition: c.line, Body: inlineBody(c)}) + out = append(out, reviewComment{Path: path, NewPosition: anchor, Body: inlineBody(c)}) if len(out) >= maxInlineComments { break } @@ -104,6 +105,25 @@ func inlineComments(clusters []cluster, addable map[string]map[int]bool) []revie return out } +// anchorLine returns the first line in [lo,hi] that is an added line in the diff, +// or 0 if none. Scanning the cluster's whole span (not just its representative +// line) anchors a finding whose min line is just outside the diff but whose span +// still overlaps a changed line. +func anchorLine(added map[int]bool, lo, hi int) int { + if added == nil || lo <= 0 { + return 0 + } + if hi < lo { // single-line cluster (maxLine unset) + hi = lo + } + for ln := lo; ln <= hi; ln++ { + if added[ln] { + return ln + } + } + return 0 +} + // inlineBody renders one inline comment: severity + title, who flagged it, detail. func inlineBody(c cluster) string { var b strings.Builder @@ -116,11 +136,13 @@ func inlineBody(c cluster) string { return b.String() } -// parseDiffNewLines returns, per file, the set of NEW-file line numbers that are -// present in the unified diff (added '+' or context ' ' lines) — the lines an -// inline comment can anchor to. Hunk lengths from the @@ header bound each hunk, -// so a content line that happens to start with "+++ " or "@@" is still read as -// content, not mistaken for a header. +// parseDiffNewLines returns, per file, the set of NEW-file line numbers that were +// ADDED in the unified diff — the safest lines for an inline comment to anchor to +// (Gitea reliably accepts comments on added lines). Context lines are walked to +// keep the line counter correct but are NOT recorded: anchoring only to added +// lines avoids the all-or-nothing review POST being rejected for an off-change +// anchor. Hunk lengths from the @@ header bound each hunk, so a content line that +// happens to start with "+++ " or "@@" is still read as content, not a header. func parseDiffNewLines(diff string) map[string]map[int]bool { out := map[string]map[int]bool{} var file string @@ -130,14 +152,13 @@ func parseDiffNewLines(diff string) map[string]map[int]bool { if inHunk && (newRem > 0 || oldRem > 0) { switch { case strings.HasPrefix(line, "+"): - record(out, file, newLine) + record(out, file, newLine) // added line — anchorable newLine++ newRem-- case strings.HasPrefix(line, "-"): oldRem-- case strings.HasPrefix(line, "\\"): // "\ No newline at end of file" - default: // context line (leading space, or an empty line) - record(out, file, newLine) + default: // context line (leading space, or an empty line): advance, don't record newLine++ newRem-- oldRem-- @@ -149,8 +170,7 @@ func parseDiffNewLines(diff string) map[string]map[int]bool { } switch { case strings.HasPrefix(line, "+++ "): - p := strings.TrimPrefix(strings.TrimPrefix(line, "+++ "), "b/") - file = strings.TrimSpace(p) + file = normPath(strings.TrimPrefix(line, "+++ ")) if file == "/dev/null" { file = "" } @@ -170,6 +190,19 @@ func parseDiffNewLines(diff string) map[string]map[int]bool { // @@ -[,] +[,] @@ var hunkRe = regexp.MustCompile(`^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@`) +// normPath trims a unified-diff path down to a repo-relative one: strip a single +// leading "a/" or "b/" prefix (and any "./"), and surrounding whitespace. Applied +// to BOTH the diff paths and a finding's file so they match even when a model +// writes "./pkg/x.go" or the diff carries the "b/" prefix. +func normPath(p string) string { + p = strings.TrimSpace(p) + p = strings.TrimPrefix(p, "./") + if strings.HasPrefix(p, "a/") || strings.HasPrefix(p, "b/") { + p = p[2:] + } + return p +} + func record(out map[string]map[int]bool, file string, line int) { if file == "" { return @@ -195,33 +228,44 @@ func atoiOr(s string, def int) int { // deletePriorReviews removes our previous inline reviews (matched by the body // marker) so a re-run replaces rather than stacks. Best-effort and quiet. func deletePriorReviews(client *http.Client, base, token string) { - req, err := http.NewRequest(http.MethodGet, base+"?limit=50", nil) - if err != nil { - return - } - req.Header.Set("Authorization", "token "+token) - resp, err := client.Do(req) - if err != nil { - return - } - var reviews []struct { - ID int `json:"id"` - Body string `json:"body"` - } - _ = json.NewDecoder(resp.Body).Decode(&reviews) - resp.Body.Close() - for _, r := range reviews { - if !strings.Contains(r.Body, inlineReviewMarker) { - continue - } - dreq, err := http.NewRequest(http.MethodDelete, fmt.Sprintf("%s/%d", base, r.ID), nil) + const perPage = 50 + for page := 1; page <= 10; page++ { // bound the scan, but page past 50 so a stale marked review isn't missed + req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("%s?limit=%d&page=%d", base, perPage, page), nil) if err != nil { - continue + return } - dreq.Header.Set("Authorization", "token "+token) - if dresp, err := client.Do(dreq); err == nil { - io.Copy(io.Discard, dresp.Body) - dresp.Body.Close() + req.Header.Set("Authorization", "token "+token) + resp, err := client.Do(req) + if err != nil { + return + } + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + io.Copy(io.Discard, resp.Body) + resp.Body.Close() + return + } + var reviews []struct { + ID int `json:"id"` + Body string `json:"body"` + } + _ = json.NewDecoder(resp.Body).Decode(&reviews) + resp.Body.Close() + for _, r := range reviews { + if !strings.Contains(r.Body, inlineReviewMarker) { + continue + } + dreq, err := http.NewRequest(http.MethodDelete, fmt.Sprintf("%s/%d", base, r.ID), nil) + if err != nil { + continue + } + dreq.Header.Set("Authorization", "token "+token) + if dresp, err := client.Do(dreq); err == nil { + io.Copy(io.Discard, dresp.Body) + dresp.Body.Close() + } + } + if len(reviews) < perPage { + return // last page } } } diff --git a/cmd/gadfly/review_test.go b/cmd/gadfly/review_test.go index 3f51b44..36dd8ca 100644 --- a/cmd/gadfly/review_test.go +++ b/cmd/gadfly/review_test.go @@ -1,6 +1,9 @@ package main -import "testing" +import ( + "strings" + "testing" +) func TestParseDiffNewLines(t *testing.T) { diff := "diff --git a/a.go b/a.go\n" + @@ -14,18 +17,19 @@ func TestParseDiffNewLines(t *testing.T) { "+new3\n" + " line4\n" got := parseDiffNewLines(diff) - want := map[int]bool{1: true, 2: true, 3: true, 4: true} // context+added new-file lines + // Only ADDED lines anchor: new2 (line 2) and new3 (line 3). Context lines 1 + // and 4 are walked for counting but not recorded. + want := map[int]bool{2: true, 3: true} if len(got["a.go"]) != len(want) { - t.Fatalf("a.go lines = %v, want %v", got["a.go"], want) + t.Fatalf("a.go lines = %v, want %v (added-only)", got["a.go"], want) } for ln := range want { if !got["a.go"][ln] { t.Errorf("expected a.go line %d anchorable", ln) } } - // The deleted line's old number must NOT be an anchor target beyond the new set. - if got["a.go"][5] { - t.Error("line 5 should not be anchorable") + if got["a.go"][1] || got["a.go"][4] { + t.Error("context lines 1/4 should not be anchorable (added-only)") } } @@ -34,8 +38,22 @@ func TestParseDiffNewLinesContentLooksLikeHeader(t *testing.T) { // diff. Hunk-length tracking must read it as content, not a file header. diff := "--- a/x\n+++ b/x\n@@ -1,1 +1,2 @@\n ctx\n+++ weird\n" got := parseDiffNewLines(diff) - if !got["x"][1] || !got["x"][2] { - t.Errorf("want lines 1 (ctx) and 2 (added) anchorable, got %v", got["x"]) + if got["x"][1] { // context line, not recorded (added-only) + t.Errorf("line 1 is context, should not anchor: %v", got["x"]) + } + if !got["x"][2] { // the "+++ weird" added line + t.Errorf("want added line 2 anchorable, got %v", got["x"]) + } +} + +func TestAnchorLineScansSpan(t *testing.T) { + // A cluster spanning 10..14 whose min line (10) isn't in the diff but whose + // span includes added line 14 must anchor to 14, not be dropped. + added := map[string]map[int]bool{"a.go": {14: true}} + clusters := []cluster{{file: "a.go", line: 10, maxLine: 14, severity: "high", title: "t", models: set("m1"), lenses: set("x")}} + cs := inlineComments(clusters, added) + if len(cs) != 1 || cs[0].NewPosition != 14 { + t.Fatalf("want anchor at 14 via span scan, got %+v", cs) } } @@ -51,6 +69,16 @@ func TestParseDiffNewLinesMultiFile(t *testing.T) { } } +func TestInlineCommentsNormalizesPaths(t *testing.T) { + // Diff path "b/pkg/x.go" -> "pkg/x.go"; a finding written as "./pkg/x.go" must + // still anchor (both normalize to the same repo-relative path). + addable := parseDiffNewLines("--- a/pkg/x.go\n+++ b/pkg/x.go\n@@ -0,0 +1,1 @@\n+bug\n") + clusters := []cluster{{file: "./pkg/x.go", line: 1, severity: "high", title: "t", models: set("m1"), lenses: set("x")}} + if cs := inlineComments(clusters, addable); len(cs) != 1 || cs[0].Path != "pkg/x.go" { + t.Errorf("path normalization failed to anchor: %+v", cs) + } +} + func TestInlineCommentsFiltersToDiffLines(t *testing.T) { addable := map[string]map[int]bool{"a.go": {10: true, 11: true}} clusters := []cluster{ @@ -65,7 +93,7 @@ func TestInlineCommentsFiltersToDiffLines(t *testing.T) { if cs[0].Path != "a.go" || cs[0].NewPosition != 10 { t.Errorf("wrong anchor: %+v", cs[0]) } - if !containsStr(cs[0].Body, "anchored") || !containsStr(cs[0].Body, "2 model") { + if !strings.Contains(cs[0].Body, "anchored") || !strings.Contains(cs[0].Body, "2 model") { t.Errorf("inline body missing title/agreement: %q", cs[0].Body) } } @@ -77,12 +105,3 @@ func set(xs ...string) map[string]bool { } return m } - -func containsStr(s, sub string) bool { - for i := 0; i+len(sub) <= len(s); i++ { - if s[i:i+len(sub)] == sub { - return true - } - } - return false -}