From aa1d0d03aee961374282f9f9cfe2d9f01c0fbe36 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 18:15:58 +0200 Subject: [PATCH 1/7] List the threads a push leaves behind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Findings exclude outdated review threads on purpose: the code they point at is gone, and anything carrying a thread ID blocks the round until it is resolved. After a push that is every thread from the previous head — so an agent that fixed and pushed had no way to close them through crq at all. The workaround was to query GitHub's GraphQL API by hand, which is the one thing the skill tells agents never to do. crq threads lists every unresolved thread, outdated ones included, current ones first, each with the thread_id resolve and decline already take. The titles are cleaned up rather than shown raw. Both bots wrap theirs in badge images and HTML, and CodeRabbit prefixes a rubric identical on every finding, so the raw first line shows everything except what the finding says. --- README.md | 1 + cmd/crq/main.go | 28 +++++++++ internal/crq/threads.go | 105 +++++++++++++++++++++++++++++++ internal/crq/threads_test.go | 83 ++++++++++++++++++++++++ llms.txt | 7 +++ skills/coderabbit-queue/SKILL.md | 11 ++++ 6 files changed, 235 insertions(+) create mode 100644 internal/crq/threads.go create mode 100644 internal/crq/threads_test.go diff --git a/README.md b/README.md index f9d091a9..65190cf8 100644 --- a/README.md +++ b/README.md @@ -383,6 +383,7 @@ least 10 minutes of silence. crq next # ⭐ the agent loop: emit the single next action as JSON (--wait blocks) crq loop # blocking one-shot round: fire + wait + emit JSON findings crq feedback # current normalized findings as JSON, WITHOUT triggering a review +crq threads # every unresolved thread, outdated included crq resolve [...] # resolve addressed review threads crq decline [...] --reason "" [--resolve] # record why a finding is declined crq autoreview # ⭐ review ALL open PRs automatically, rate-coordinated diff --git a/cmd/crq/main.go b/cmd/crq/main.go index f83f08ae..d0597833 100644 --- a/cmd/crq/main.go +++ b/cmd/crq/main.go @@ -198,6 +198,19 @@ func run(ctx context.Context, args []string) int { } printJSON(result) return 0 + case "threads": + repo, pr, ok := repoPR(args[1:]) + if !ok { + fatal(errors.New("usage: crq threads ")) + return 1 + } + threads, terr := service.OpenThreads(ctx, repo, pr) + if terr != nil { + fatal(terr) + return 1 + } + printJSON(threads) + return 0 case "decline": threads, reason, resolve, ok := parseDeclineArgs(args[1:]) if !ok || len(threads) == 0 || strings.TrimSpace(reason) == "" { @@ -341,6 +354,7 @@ USAGE crq decline [...] --reason "" [--keep-open] reply on a thread to record why a finding is declined (resolves it; --keep-open leaves it open) + crq threads list every unresolved review thread, outdated ones included crq autoreview [--once] [--no-incremental] keep open PRs reviewed, rate-coordinated crq preflight [--type all|committed|uncommitted] [--base ] @@ -494,6 +508,20 @@ replies contesting the decline, crq re-surfaces that reply as its own finding. Pass --keep-open to leave it unresolved anyway (an on-the-record disagreement you intend to keep working). Thread IDs come from .findings[].thread_id. +`) + case "threads": + fmt.Print(`crq threads + +List the PR's unresolved review threads as JSON, including the ones GitHub has +marked OUTDATED, with .thread_id ready for crq resolve. + +Findings leave outdated threads out on purpose: the code they point at is gone, +and anything carrying a thread ID blocks the round until it is resolved. But an +outdated thread is still open on the PR — and after a push that is every thread +from the previous head, so fixing and pushing used to leave no way to close them +through crq at all. + +Read this, decide, then crq resolve (or crq decline) the ones you have answered. `) case "autoreview", "auto": fmt.Print(`crq autoreview [--once] [--no-incremental] diff --git a/internal/crq/threads.go b/internal/crq/threads.go new file mode 100644 index 00000000..13f6f09d --- /dev/null +++ b/internal/crq/threads.go @@ -0,0 +1,105 @@ +package crq + +import ( + "context" + "regexp" + "sort" + "strings" + + "github.com/kristofferR/coderabbit-queue/internal/dialect" +) + +// OpenThread is one unresolved review thread, enough to decide about it and to +// pass its ID to `crq resolve`. +type OpenThread struct { + ID string `json:"thread_id"` + Path string `json:"path,omitempty"` + Line int `json:"line,omitempty"` + Bot string `json:"bot,omitempty"` + Outdated bool `json:"outdated"` + // Title is the thread's first line, so a list is readable without opening + // each one. + Title string `json:"title,omitempty"` + URL string `json:"url,omitempty"` +} + +// OpenThreads lists a PR's unresolved review threads, INCLUDING the ones GitHub +// has marked outdated. +// +// Findings deliberately exclude outdated threads: the code they point at is +// gone, so re-reporting them would be noise, and anything with a thread ID +// blocks a round until it is resolved. But an outdated thread is still open on +// the PR, and after a push that is every thread from the previous head — so an +// agent that fixed and pushed had no way left to close them through crq at all. +// The documented answer was to stop using crq and query GitHub's GraphQL API by +// hand, which is the one thing the skill tells agents never to do. +// +// This is the read that closes that loop: list, decide, `crq resolve`. +func (s *Service) OpenThreads(ctx context.Context, repo string, pr int) ([]OpenThread, error) { + threads, err := s.reviewThreads(ctx, repo, pr) + if err != nil { + return nil, err + } + out := make([]OpenThread, 0, len(threads)) + for _, thread := range threads { + if thread.IsResolved { + continue + } + open := OpenThread{ID: thread.ID, Path: thread.Path, Line: thread.Line, Outdated: thread.IsOutdated} + if nodes := thread.Comments.Nodes; len(nodes) > 0 { + first := nodes[0] + open.Bot = dialect.NormalizeBotName(first.Author.Login) + open.URL = first.URL + open.Title = threadTitle(first.Body) + if open.Path == "" { + open.Path = first.Path + } + if open.Line == 0 { + open.Line = first.Line + } + } + out = append(out, open) + } + // Current threads first, then by location, so the ones still pointing at + // live code are what a reader sees first. + sort.SliceStable(out, func(i, j int) bool { + if out[i].Outdated != out[j].Outdated { + return !out[i].Outdated + } + if out[i].Path != out[j].Path { + return out[i].Path < out[j].Path + } + return out[i].Line < out[j].Line + }) + return out, nil +} + +// markup matches the badge images, HTML wrappers and emphasis a bot puts around +// its title. Left in, a listing reads as `![P2 Badge](https://img.shields.io/...)` +// instead of what the finding says. +var markup = regexp.MustCompile(`!\[[^\]]*\]\([^)]*\)|<[^>]+>|[*_` + "`" + `#]+`) + +// threadTitle reduces a comment body to one readable line. It prefers the bot's +// own bold title, which is where both CodeRabbit and Codex put the summary. +func threadTitle(body string) string { + title := dialect.TitleFromDetailedBlock(body) + if strings.TrimSpace(markup.ReplaceAllString(title, "")) == "" { + title = dialect.TitleOf(body) + } + title = strings.Join(strings.Fields(markup.ReplaceAllString(title, " ")), " ") + // Drop a leading severity/category rubric ("Functional Correctness | Minor | + // Quick win"): it is the same on every finding, so it displaces the part that + // differs. + if _, rest, ok := strings.Cut(title, "|"); ok { + if last := strings.LastIndex(rest, "|"); last >= 0 { + rest = rest[last+1:] + } + if trimmed := strings.TrimSpace(rest); trimmed != "" { + title = trimmed + } + } + if len(title) > 120 { + title = title[:117] + "..." + } + return title +} diff --git a/internal/crq/threads_test.go b/internal/crq/threads_test.go new file mode 100644 index 00000000..c8f4c303 --- /dev/null +++ b/internal/crq/threads_test.go @@ -0,0 +1,83 @@ +package crq + +import ( + "context" + "encoding/json" + "strings" + "testing" +) + +// A listing exists to be read. Both bots wrap their title in badge images and +// HTML, and CodeRabbit prefixes a rubric that is identical on every finding, so +// the raw first line shows everything except what the finding says. +func TestThreadTitleReadsAsText(t *testing.T) { + for _, tc := range []struct { + name string + body string + want string + }{ + { + "codex badge", + "**![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat) Remove the primary from the legacy view**\n\nWhen CRQ_BOT names a registry bot...", + "Remove the primary from the legacy view", + }, + { + "coderabbit rubric", + "_🎯 Functional Correctness_ | _🟡 Minor_ | _⚡ Quick win_\n\n**Reject flags supplied as option values.**\n\nDetail follows.", + "Reject flags supplied as option values.", + }, + {"plain", "Just a sentence about the bug.", "Just a sentence about the bug."}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := threadTitle(tc.body); got != tc.want { + t.Errorf("threadTitle = %q, want %q", got, tc.want) + } + }) + } + + long := threadTitle(strings.Repeat("x", 400)) + if len(long) > 120 { + t.Errorf("title is %d chars; a listing line must stay short", len(long)) + } +} + +// The point of the command: after a push every thread from the previous head is +// outdated, findings leave those out by design, and an agent then had no way to +// close them through crq at all. They must be listed — and listed last, since +// the threads still pointing at live code matter more. +func TestOpenThreadsIncludesOutdatedAndOrdersThem(t *testing.T) { + gh := newFakeGitHub() + gh.graphQL = func(query string, _ map[string]any, out any) error { + if !strings.Contains(query, "reviewThreads") { + return nil + } + return json.Unmarshal([]byte(`{"repository":{"pullRequest":{"reviewThreads":{ + "pageInfo":{"hasNextPage":false,"endCursor":""}, + "nodes":[ + {"id":"T_outdated","isResolved":false,"isOutdated":true,"path":"a.go","line":1, + "comments":{"totalCount":1,"nodes":[{"body":"**Stale point**","author":{"login":"coderabbitai[bot]"}}]}}, + {"id":"T_resolved","isResolved":true,"isOutdated":false,"path":"b.go","line":2, + "comments":{"totalCount":1,"nodes":[{"body":"**Done**","author":{"login":"coderabbitai[bot]"}}]}}, + {"id":"T_current","isResolved":false,"isOutdated":false,"path":"c.go","line":3, + "comments":{"totalCount":1,"nodes":[{"body":"**Live point**","author":{"login":"coderabbitai[bot]"}}]}} + ]}}}}`), out) + } + svc := NewService(firingConfig(), gh, NewMemoryStore(firingConfig()), nil) + + threads, err := svc.OpenThreads(context.Background(), "o/r", 1) + if err != nil { + t.Fatal(err) + } + if len(threads) != 2 { + t.Fatalf("got %d threads, want the two unresolved ones", len(threads)) + } + if threads[0].ID != "T_current" || threads[0].Outdated { + t.Errorf("first = %+v, want the current thread", threads[0]) + } + if threads[1].ID != "T_outdated" || !threads[1].Outdated { + t.Errorf("second = %+v, want the outdated thread — omitting it is the bug", threads[1]) + } + if threads[0].Title != "Live point" || threads[0].Bot != "coderabbitai" { + t.Errorf("thread = %+v, want a readable title and the bot named", threads[0]) + } +} diff --git a/llms.txt b/llms.txt index f16ddff6..86c5be5a 100644 --- a/llms.txt +++ b/llms.txt @@ -186,6 +186,13 @@ reappearing until its thread is resolved there): crq resolve THREAD_ID [THREAD_ID...] ``` +After a push, threads from the previous head become outdated and drop out of `findings`, so their +IDs are no longer available there. List every unresolved thread, outdated included: + +```bash +crq threads REPO PR +``` + Decline a finding you are not addressing. This posts the reason AND resolves the thread — crq reads GitHub resolution state, so one left open keeps returning `fix`. A contested bot reply is re-surfaced as its own finding, so nothing is buried. Use `--keep-open` to leave it open on purpose: diff --git a/skills/coderabbit-queue/SKILL.md b/skills/coderabbit-queue/SKILL.md index cab6ef8b..1ce0f073 100644 --- a/skills/coderabbit-queue/SKILL.md +++ b/skills/coderabbit-queue/SKILL.md @@ -158,6 +158,17 @@ call rather than looping a subprocess per thread. crq keys off GitHub's resolution state: an addressed finding keeps reappearing in `crq feedback` until its thread is resolved on GitHub. Resolve only threads you actually addressed; leave the rest open. +After a push, every thread from the previous head is **outdated**. Findings leave those out on +purpose — the code they point at is gone — so `crq feedback` no longer gives you their IDs even +though they are still open on the PR. List them instead of reaching for the GitHub API: + +```bash +crq threads "$REPO" "$PR" +``` + +It returns every unresolved thread, outdated ones included, current ones first, each with the +`thread_id` that `crq resolve` and `crq decline` take. + For a finding you are **not** addressing, record why instead of leaving it silently open: ```bash From d5f0eaa472a584475f65944eda8a86011d07b423 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 19:37:12 +0200 Subject: [PATCH 2/7] Keep a title's own characters out of the markup stripping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four review findings, all correct. Stripping every underscore, backtick, asterisk and hash turned "Handle user_id" into "Handle user id" — a different identifier. Emphasis is now matched only where it delimits a span, at a boundary, so markup goes and content stays. Any pipe was treated as CodeRabbit's severity rubric, so "Support A | B configuration" became "B configuration". The rubric is matched by its shape now, not by containing a separator a title is allowed to use. Truncation cut bytes, which can split a multi-byte character and leave invalid UTF-8 that JSON renders as a replacement character. It cuts runes. And `crq threads` skipped RequireState, so it failed inside the API instead of saying the configuration was missing. --- cmd/crq/main.go | 4 ++++ internal/crq/threads.go | 40 +++++++++++++++++++++--------------- internal/crq/threads_test.go | 27 ++++++++++++++++++++++-- 3 files changed, 52 insertions(+), 19 deletions(-) diff --git a/cmd/crq/main.go b/cmd/crq/main.go index d0597833..11e23439 100644 --- a/cmd/crq/main.go +++ b/cmd/crq/main.go @@ -204,6 +204,10 @@ func run(ctx context.Context, args []string) int { fatal(errors.New("usage: crq threads ")) return 1 } + if err := cfg.RequireState(); err != nil { + fatal(err) + return 1 + } threads, terr := service.OpenThreads(ctx, repo, pr) if terr != nil { fatal(terr) diff --git a/internal/crq/threads.go b/internal/crq/threads.go index 13f6f09d..91456e35 100644 --- a/internal/crq/threads.go +++ b/internal/crq/threads.go @@ -74,32 +74,38 @@ func (s *Service) OpenThreads(ctx context.Context, repo string, pr int) ([]OpenT return out, nil } -// markup matches the badge images, HTML wrappers and emphasis a bot puts around -// its title. Left in, a listing reads as `![P2 Badge](https://img.shields.io/...)` -// instead of what the finding says. -var markup = regexp.MustCompile(`!\[[^\]]*\]\([^)]*\)|<[^>]+>|[*_` + "`" + `#]+`) +// markup matches the wrappers a bot puts AROUND its title: badge images, HTML +// tags, and emphasis runs that delimit a span. Left in, a listing reads as +// `![P2 Badge](https://img.shields.io/...)` instead of what the finding says. +// +// Emphasis is matched only where it delimits — at a boundary — so a literal +// underscore inside a word survives. Stripping every `_` turned "Handle user_id" +// into "Handle user id", which is a different identifier. +var markup = regexp.MustCompile(`!\[[^\]]*\]\([^)]*\)|<[^>]+>|(^|\s)[*_` + "`" + `#]+|[*_` + "`" + `#]+(\s|$)`) + +// rubric matches CodeRabbit's fixed severity header — "🎯 Functional +// Correctness | 🟡 Minor | ⚡ Quick win" — which is the same on every finding +// and displaces the part that differs. +// +// It matches the SHAPE rather than any pipe, because a title is allowed to +// contain one: "Support A | B configuration" must not become "B configuration". +var rubric = regexp.MustCompile(`^[^|]*\b(Correctness|Maintainability|Security|Performance|Reliability|Quality)\b[^|]*\|[^|]*\|[^|]*\|?\s*`) // threadTitle reduces a comment body to one readable line. It prefers the bot's // own bold title, which is where both CodeRabbit and Codex put the summary. func threadTitle(body string) string { title := dialect.TitleFromDetailedBlock(body) - if strings.TrimSpace(markup.ReplaceAllString(title, "")) == "" { + if strings.TrimSpace(markup.ReplaceAllString(title, " ")) == "" { title = dialect.TitleOf(body) } title = strings.Join(strings.Fields(markup.ReplaceAllString(title, " ")), " ") - // Drop a leading severity/category rubric ("Functional Correctness | Minor | - // Quick win"): it is the same on every finding, so it displaces the part that - // differs. - if _, rest, ok := strings.Cut(title, "|"); ok { - if last := strings.LastIndex(rest, "|"); last >= 0 { - rest = rest[last+1:] - } - if trimmed := strings.TrimSpace(rest); trimmed != "" { - title = trimmed - } + if trimmed := strings.TrimSpace(rubric.ReplaceAllString(title, "")); trimmed != "" { + title = trimmed } - if len(title) > 120 { - title = title[:117] + "..." + // By runes: cutting bytes can split a multi-byte character, and the invalid + // UTF-8 that leaves becomes a replacement character in the JSON. + if runes := []rune(title); len(runes) > 120 { + title = string(runes[:117]) + "..." } return title } diff --git a/internal/crq/threads_test.go b/internal/crq/threads_test.go index c8f4c303..b40d096b 100644 --- a/internal/crq/threads_test.go +++ b/internal/crq/threads_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "strings" "testing" + "unicode/utf8" ) // A listing exists to be read. Both bots wrap their title in badge images and @@ -27,6 +28,18 @@ func TestThreadTitleReadsAsText(t *testing.T) { "Reject flags supplied as option values.", }, {"plain", "Just a sentence about the bug.", "Just a sentence about the bug."}, + { + // A pipe is allowed in a title; only the fixed rubric header goes. + "pipe in the title", + "**Support A | B configuration**\n\nDetail.", + "Support A | B configuration", + }, + { + // Underscores inside an identifier are content, not emphasis. + "identifier with underscores", + "**Handle user_id and api_key**\n\nDetail.", + "Handle user_id and api_key", + }, } { t.Run(tc.name, func(t *testing.T) { if got := threadTitle(tc.body); got != tc.want { @@ -36,8 +49,18 @@ func TestThreadTitleReadsAsText(t *testing.T) { } long := threadTitle(strings.Repeat("x", 400)) - if len(long) > 120 { - t.Errorf("title is %d chars; a listing line must stay short", len(long)) + if len([]rune(long)) > 120 { + t.Errorf("title is %d chars; a listing line must stay short", len([]rune(long))) + } + + // Truncation cuts runes, not bytes: splitting a multi-byte character leaves + // invalid UTF-8, which JSON renders as a replacement character. + wide := threadTitle(strings.Repeat("é", 400)) + if !utf8.ValidString(wide) { + t.Errorf("truncated title is not valid UTF-8: %q", wide) + } + if strings.Contains(wide, "\uFFFD") { + t.Errorf("truncation split a character: %q", wide) } } From 84138515b9b8737942bbb4331ffd5f7c2e49b1ad Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 19:48:32 +0200 Subject: [PATCH 3/7] Put the title parsing where bot wording belongs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings, one P1 about this repository's own rule: all bot-text knowledge lives in dialect. The badge stripping and rubric handling had encoded four bots' comment formats inside the orchestration layer, where a wording change would bypass the corpus and golden-classification workflow entirely. ThreadTitle moves to dialect, and moving it exposed what living there means: the first bold span is the SEVERITY for Bugbot and Macroscope, so every one of their threads was listed as "High Severity" or "High". It now prefers a heading for the bots that lead with a severity label, and rejects a title that is only a severity word whatever the source. Two smaller ones. The command promises every unresolved thread, humans included, and it copied the author into `bot` — so a person's thread was serialized as `"bot":"alice"`. A bot is named only when the author is one; a human is named as the author. And an outdated thread often keeps only originalLine, which is exactly the case this command exists for, so dropping the location lost the only pointer to what the comment was about. --- internal/crq/threads.go | 72 ++++++++++------------- internal/crq/threads_test.go | 101 ++++++++++++++------------------- internal/dialect/title.go | 85 +++++++++++++++++++++++++++ internal/dialect/title_test.go | 91 +++++++++++++++++++++++++++++ 4 files changed, 251 insertions(+), 98 deletions(-) create mode 100644 internal/dialect/title.go create mode 100644 internal/dialect/title_test.go diff --git a/internal/crq/threads.go b/internal/crq/threads.go index 91456e35..8af23d62 100644 --- a/internal/crq/threads.go +++ b/internal/crq/threads.go @@ -2,9 +2,7 @@ package crq import ( "context" - "regexp" "sort" - "strings" "github.com/kristofferR/coderabbit-queue/internal/dialect" ) @@ -12,10 +10,12 @@ import ( // OpenThread is one unresolved review thread, enough to decide about it and to // pass its ID to `crq resolve`. type OpenThread struct { - ID string `json:"thread_id"` - Path string `json:"path,omitempty"` - Line int `json:"line,omitempty"` - Bot string `json:"bot,omitempty"` + ID string `json:"thread_id"` + Path string `json:"path,omitempty"` + Line int `json:"line,omitempty"` + Bot string `json:"bot,omitempty"` + // Author names a human reviewer, when the thread is not a bot's. + Author string `json:"author,omitempty"` Outdated bool `json:"outdated"` // Title is the thread's first line, so a list is readable without opening // each one. @@ -48,14 +48,24 @@ func (s *Service) OpenThreads(ctx context.Context, repo string, pr int) ([]OpenT open := OpenThread{ID: thread.ID, Path: thread.Path, Line: thread.Line, Outdated: thread.IsOutdated} if nodes := thread.Comments.Nodes; len(nodes) > 0 { first := nodes[0] - open.Bot = dialect.NormalizeBotName(first.Author.Login) + // Only when the author is actually a reviewer bot. The command + // promises every unresolved thread, humans included, and labelling + // "alice" as a bot makes the output plainly wrong. + if login := first.Author.Login; isReviewerBot(s.cfg, login) { + open.Bot = dialect.NormalizeBotName(login) + } else { + open.Author = login + } open.URL = first.URL - open.Title = threadTitle(first.Body) + open.Title = dialect.ThreadTitle(first.Author.Login, first.Body) if open.Path == "" { open.Path = first.Path } if open.Line == 0 { - open.Line = first.Line + // GitHub clears both current lines on an outdated thread while + // keeping originalLine — and outdated threads are the whole reason + // this command exists. + open.Line = firstPositive(first.Line, first.OriginalLine) } } out = append(out, open) @@ -74,38 +84,18 @@ func (s *Service) OpenThreads(ctx context.Context, repo string, pr int) ([]OpenT return out, nil } -// markup matches the wrappers a bot puts AROUND its title: badge images, HTML -// tags, and emphasis runs that delimit a span. Left in, a listing reads as -// `![P2 Badge](https://img.shields.io/...)` instead of what the finding says. -// -// Emphasis is matched only where it delimits — at a boundary — so a literal -// underscore inside a word survives. Stripping every `_` turned "Handle user_id" -// into "Handle user id", which is a different identifier. -var markup = regexp.MustCompile(`!\[[^\]]*\]\([^)]*\)|<[^>]+>|(^|\s)[*_` + "`" + `#]+|[*_` + "`" + `#]+(\s|$)`) - -// rubric matches CodeRabbit's fixed severity header — "🎯 Functional -// Correctness | 🟡 Minor | ⚡ Quick win" — which is the same on every finding -// and displaces the part that differs. -// -// It matches the SHAPE rather than any pipe, because a title is allowed to -// contain one: "Support A | B configuration" must not become "B configuration". -var rubric = regexp.MustCompile(`^[^|]*\b(Correctness|Maintainability|Security|Performance|Reliability|Quality)\b[^|]*\|[^|]*\|[^|]*\|?\s*`) - -// threadTitle reduces a comment body to one readable line. It prefers the bot's -// own bold title, which is where both CodeRabbit and Codex put the summary. -func threadTitle(body string) string { - title := dialect.TitleFromDetailedBlock(body) - if strings.TrimSpace(markup.ReplaceAllString(title, " ")) == "" { - title = dialect.TitleOf(body) - } - title = strings.Join(strings.Fields(markup.ReplaceAllString(title, " ")), " ") - if trimmed := strings.TrimSpace(rubric.ReplaceAllString(title, "")); trimmed != "" { - title = trimmed +// isReviewerBot reports whether login is a reviewer crq knows: the configured +// primary, a configured co-reviewer, or a registry entry. +func isReviewerBot(cfg Config, login string) bool { + key := dialect.NormalizeBotName(login) + if key == dialect.NormalizeBotName(cfg.Bot) { + return true } - // By runes: cutting bytes can split a multi-byte character, and the invalid - // UTF-8 that leaves becomes a replacement character in the JSON. - if runes := []rune(title); len(runes) > 120 { - title = string(runes[:117]) + "..." + for _, cb := range cfg.CoBots { + if dialect.NormalizeBotName(cb.Login) == key { + return true + } } - return title + _, known := dialect.CoReviewerByName(login) + return known } diff --git a/internal/crq/threads_test.go b/internal/crq/threads_test.go index b40d096b..998155f7 100644 --- a/internal/crq/threads_test.go +++ b/internal/crq/threads_test.go @@ -5,65 +5,8 @@ import ( "encoding/json" "strings" "testing" - "unicode/utf8" ) -// A listing exists to be read. Both bots wrap their title in badge images and -// HTML, and CodeRabbit prefixes a rubric that is identical on every finding, so -// the raw first line shows everything except what the finding says. -func TestThreadTitleReadsAsText(t *testing.T) { - for _, tc := range []struct { - name string - body string - want string - }{ - { - "codex badge", - "**![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat) Remove the primary from the legacy view**\n\nWhen CRQ_BOT names a registry bot...", - "Remove the primary from the legacy view", - }, - { - "coderabbit rubric", - "_🎯 Functional Correctness_ | _🟡 Minor_ | _⚡ Quick win_\n\n**Reject flags supplied as option values.**\n\nDetail follows.", - "Reject flags supplied as option values.", - }, - {"plain", "Just a sentence about the bug.", "Just a sentence about the bug."}, - { - // A pipe is allowed in a title; only the fixed rubric header goes. - "pipe in the title", - "**Support A | B configuration**\n\nDetail.", - "Support A | B configuration", - }, - { - // Underscores inside an identifier are content, not emphasis. - "identifier with underscores", - "**Handle user_id and api_key**\n\nDetail.", - "Handle user_id and api_key", - }, - } { - t.Run(tc.name, func(t *testing.T) { - if got := threadTitle(tc.body); got != tc.want { - t.Errorf("threadTitle = %q, want %q", got, tc.want) - } - }) - } - - long := threadTitle(strings.Repeat("x", 400)) - if len([]rune(long)) > 120 { - t.Errorf("title is %d chars; a listing line must stay short", len([]rune(long))) - } - - // Truncation cuts runes, not bytes: splitting a multi-byte character leaves - // invalid UTF-8, which JSON renders as a replacement character. - wide := threadTitle(strings.Repeat("é", 400)) - if !utf8.ValidString(wide) { - t.Errorf("truncated title is not valid UTF-8: %q", wide) - } - if strings.Contains(wide, "\uFFFD") { - t.Errorf("truncation split a character: %q", wide) - } -} - // The point of the command: after a push every thread from the previous head is // outdated, findings leave those out by design, and an agent then had no way to // close them through crq at all. They must be listed — and listed last, since @@ -103,4 +46,48 @@ func TestOpenThreadsIncludesOutdatedAndOrdersThem(t *testing.T) { if threads[0].Title != "Live point" || threads[0].Bot != "coderabbitai" { t.Errorf("thread = %+v, want a readable title and the bot named", threads[0]) } + // A human's thread is listed too — the command promises every unresolved one + // — but calling a person a bot makes the output plainly wrong. + if threads[0].Author != "" { + t.Errorf("thread = %+v, want no author on a bot's thread", threads[0]) + } +} + +// The command promises every unresolved thread, so it also gets humans'. Copying +// the author into `bot` made the output say `"bot":"alice"`. +func TestOpenThreadsDoesNotCallAPersonABot(t *testing.T) { + gh := newFakeGitHub() + gh.graphQL = func(query string, _ map[string]any, out any) error { + if !strings.Contains(query, "reviewThreads") { + return nil + } + return json.Unmarshal([]byte(`{"repository":{"pullRequest":{"reviewThreads":{ + "pageInfo":{"hasNextPage":false,"endCursor":""}, + "nodes":[{"id":"T_human","isResolved":false,"isOutdated":true,"path":"","line":0, + "comments":{"totalCount":1,"nodes":[{"body":"Can we rename this?","originalLine":42, + "path":"a.go","author":{"login":"alice"}}]}}] + }}}}`), out) + } + cfg := firingConfig() + svc := NewService(cfg, gh, NewMemoryStore(cfg), nil) + + threads, err := svc.OpenThreads(context.Background(), "o/r", 1) + if err != nil { + t.Fatal(err) + } + if len(threads) != 1 { + t.Fatalf("got %d threads, want the human's", len(threads)) + } + if threads[0].Bot != "" { + t.Errorf("thread = %+v, want no bot on a human's thread", threads[0]) + } + if threads[0].Author != "alice" { + t.Errorf("thread = %+v, want the human named as the author", threads[0]) + } + // An outdated thread often keeps only originalLine, and those are the whole + // reason this command exists — dropping the location loses the only pointer + // to what the comment was about. + if threads[0].Line != 42 { + t.Errorf("line = %d, want the original line 42", threads[0].Line) + } } diff --git a/internal/dialect/title.go b/internal/dialect/title.go new file mode 100644 index 00000000..4816df8c --- /dev/null +++ b/internal/dialect/title.go @@ -0,0 +1,85 @@ +package dialect + +import ( + "regexp" + "strings" +) + +// markup matches the wrappers a bot puts AROUND its title: badge images, HTML +// tags, and emphasis runs that delimit a span. Left in, a listing reads as +// `![P2 Badge](https://img.shields.io/...)` instead of what the finding says. +// +// Emphasis is matched only where it delimits — at a boundary — so a literal +// underscore inside a word survives. Stripping every `_` turned "Handle user_id" +// into "Handle user id", which is a different identifier. +var markup = regexp.MustCompile(`!\[[^\]]*\]\([^)]*\)|<[^>]+>|(^|\s)[*_` + "`" + `#]+|[*_` + "`" + `#]+(\s|$)`) + +// rubric matches CodeRabbit's fixed severity header — "🎯 Functional +// Correctness | 🟡 Minor | ⚡ Quick win" — which is the same on every finding +// and displaces the part that differs. It matches the SHAPE rather than any +// pipe, because a title may contain one: "Support A | B configuration" must not +// become "B configuration". +var rubric = regexp.MustCompile(`^[^|]*\b(Correctness|Maintainability|Security|Performance|Reliability|Quality)\b[^|]*\|[^|]*\|[^|]*\|?\s*`) + +// severityOnly matches a title that is just a severity label — "High Severity", +// "🟠 High", "Critical" — which is what the first bold span is for Bugbot and +// Macroscope. It carries no information a severity field does not already hold. +var severityOnly = regexp.MustCompile(`(?i)^\W*(critical|high|medium|low|major|minor|blocker|trivial|info)(\s+severity)?\W*$`) + +// ThreadTitle reduces a review comment to one readable line, per bot. +// +// Every bot wraps its summary differently: CodeRabbit prefixes a fixed severity +// rubric, Codex leads with a badge image, Bugbot opens with a `###` heading +// followed by a bold severity, and Macroscope starts with an emoji severity and +// the path. Taking the first bold span works for two of them and reports "High +// Severity" for the others. +// +// It lives here because this is where bot wording lives; a listing in the +// orchestration layer must not have to know how a bot writes a heading. +func ThreadTitle(author, body string) string { + candidates := []string{TitleFromDetailedBlock(body)} + if co, ok := CoReviewerByName(author); ok && co.Name != "" { + // A heading, where the bots that lead with a severity label put the + // actual summary. + candidates = append([]string{headingOf(body)}, candidates...) + } + candidates = append(candidates, TitleOf(body)) + + for _, candidate := range candidates { + title := cleanTitle(candidate) + if title != "" && !severityOnly.MatchString(title) { + return title + } + } + // Everything looked like a severity label; the least bad answer is the + // first non-empty one rather than nothing at all. + for _, candidate := range candidates { + if title := cleanTitle(candidate); title != "" { + return title + } + } + return "" +} + +// headingOf returns the first Markdown heading's text. +func headingOf(body string) string { + for _, line := range strings.Split(body, "\n") { + if trimmed := strings.TrimSpace(line); strings.HasPrefix(trimmed, "#") { + return strings.TrimSpace(strings.TrimLeft(trimmed, "# ")) + } + } + return "" +} + +func cleanTitle(title string) string { + title = strings.Join(strings.Fields(markup.ReplaceAllString(title, " ")), " ") + if trimmed := strings.TrimSpace(rubric.ReplaceAllString(title, "")); trimmed != "" { + title = trimmed + } + // By runes: cutting bytes can split a multi-byte character, and the invalid + // UTF-8 that leaves becomes a replacement character in the JSON. + if runes := []rune(title); len(runes) > 120 { + title = string(runes[:117]) + "..." + } + return strings.TrimSpace(title) +} diff --git a/internal/dialect/title_test.go b/internal/dialect/title_test.go new file mode 100644 index 00000000..95bedadb --- /dev/null +++ b/internal/dialect/title_test.go @@ -0,0 +1,91 @@ +package dialect + +import ( + "strings" + "testing" + "unicode/utf8" +) + +// A listing exists to be read. Both bots wrap their title in badge images and +// HTML, and CodeRabbit prefixes a rubric that is identical on every finding, so +// the raw first line shows everything except what the finding says. +func TestThreadTitleReadsAsText(t *testing.T) { + for _, tc := range []struct { + name string + body string + want string + }{ + { + "codex badge", + "**![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat) Remove the primary from the legacy view**\n\nWhen CRQ_BOT names a registry bot...", + "Remove the primary from the legacy view", + }, + { + "coderabbit rubric", + "_🎯 Functional Correctness_ | _🟡 Minor_ | _⚡ Quick win_\n\n**Reject flags supplied as option values.**\n\nDetail follows.", + "Reject flags supplied as option values.", + }, + {"plain", "Just a sentence about the bug.", "Just a sentence about the bug."}, + { + // A pipe is allowed in a title; only the fixed rubric header goes. + "pipe in the title", + "**Support A | B configuration**\n\nDetail.", + "Support A | B configuration", + }, + { + // Underscores inside an identifier are content, not emphasis. + "identifier with underscores", + "**Handle user_id and api_key**\n\nDetail.", + "Handle user_id and api_key", + }, + } { + t.Run(tc.name, func(t *testing.T) { + if got := ThreadTitle("coderabbitai[bot]", tc.body); got != tc.want { + t.Errorf("threadTitle = %q, want %q", got, tc.want) + } + }) + } + + long := ThreadTitle("coderabbitai[bot]", strings.Repeat("x", 400)) + if len([]rune(long)) > 120 { + t.Errorf("title is %d chars; a listing line must stay short", len([]rune(long))) + } + + // Truncation cuts runes, not bytes: splitting a multi-byte character leaves + // invalid UTF-8, which JSON renders as a replacement character. + wide := ThreadTitle("coderabbitai[bot]", strings.Repeat("é", 400)) + if !utf8.ValidString(wide) { + t.Errorf("truncated title is not valid UTF-8: %q", wide) + } + if strings.Contains(wide, "\uFFFD") { + t.Errorf("truncation split a character: %q", wide) + } +} + +// Every bot wraps its summary differently, and the first bold span is the +// SEVERITY for two of them — a listing that says "High Severity" for every +// Bugbot finding has told the reader nothing. +func TestThreadTitleSkipsASeverityLabel(t *testing.T) { + for _, tc := range []struct { + name, author, body, want string + }{ + { + "bugbot heading then severity", + "cursor[bot]", + "### Nil map write in the claim path\n\n**High Severity**\n\nDetail follows.", + "Nil map write in the claim path", + }, + { + "macroscope severity first", + "macroscopeapp[bot]", + "🟠 **High** internal/crq/service.go\n\n### Slot released twice\n\nDetail.", + "Slot released twice", + }, + } { + t.Run(tc.name, func(t *testing.T) { + if got := ThreadTitle(tc.author, tc.body); got != tc.want { + t.Errorf("ThreadTitle = %q, want %q", got, tc.want) + } + }) + } +} From f0d5267510b44f4671b07366c812876f983d8e8e Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 21:39:57 +0200 Subject: [PATCH 4/7] Read each bot's title the way it actually writes one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six findings, all about the title reading as something a person can use. Against the real corpus, Bugbot's first bold span is "High Severity" and Macroscope's first line is a severity and a path — so both listed every finding under a label that says nothing. The extraction now prefers a heading, then the bold span, then the first line that reads like prose, and rejects a candidate that is only a severity word or only a file reference. The tests run against the captured comments rather than my idea of them. Whether the author is a reviewer is the CALLER's question now. dialect knows co-reviewers, not the configured primary, and CoReviewerByName also matches the config name — so a human whose GitHub handle is "codex" was serialized as that bot. crq answers it by login. The markup stripping was eating real content: a generic <[^>]+> removed `Map` down to "Map", and stripping every # turned "#123" into "123". Only the tags the bots actually use, and emphasis only where it delimits. A person's first line is what they meant to say, so a label they bolded later no longer wins over it. And TitleOf caps at 180 BYTES, which can leave half a rune for JSON to render as a replacement character. --- internal/crq/threads.go | 17 ++++-- internal/dialect/title.go | 106 ++++++++++++++++++++++++++------- internal/dialect/title_test.go | 58 ++++++++++++++++-- 3 files changed, 148 insertions(+), 33 deletions(-) diff --git a/internal/crq/threads.go b/internal/crq/threads.go index 8af23d62..e38e0d1f 100644 --- a/internal/crq/threads.go +++ b/internal/crq/threads.go @@ -51,13 +51,14 @@ func (s *Service) OpenThreads(ctx context.Context, repo string, pr int) ([]OpenT // Only when the author is actually a reviewer bot. The command // promises every unresolved thread, humans included, and labelling // "alice" as a bot makes the output plainly wrong. - if login := first.Author.Login; isReviewerBot(s.cfg, login) { - open.Bot = dialect.NormalizeBotName(login) + isBot := isReviewerBot(s.cfg, first.Author.Login) + if isBot { + open.Bot = dialect.NormalizeBotName(first.Author.Login) } else { - open.Author = login + open.Author = first.Author.Login } + open.Title = dialect.ThreadTitle(isBot, first.Body) open.URL = first.URL - open.Title = dialect.ThreadTitle(first.Author.Login, first.Body) if open.Path == "" { open.Path = first.Path } @@ -96,6 +97,10 @@ func isReviewerBot(cfg Config, login string) bool { return true } } - _, known := dialect.CoReviewerByName(login) - return known + // By login. CoReviewerByName also matches the config NAME, so a human whose + // GitHub handle is "codex" or "bugbot" would be serialized as that bot. + if co, ok := dialect.CoReviewerByName(login); ok { + return dialect.NormalizeBotName(co.Login) == key + } + return false } diff --git a/internal/dialect/title.go b/internal/dialect/title.go index 4816df8c..00a6b441 100644 --- a/internal/dialect/title.go +++ b/internal/dialect/title.go @@ -5,14 +5,16 @@ import ( "strings" ) -// markup matches the wrappers a bot puts AROUND its title: badge images, HTML -// tags, and emphasis runs that delimit a span. Left in, a listing reads as -// `![P2 Badge](https://img.shields.io/...)` instead of what the finding says. +// markup matches the wrappers a bot puts AROUND its title: badge images, the +// specific HTML tags the bots use, and emphasis runs that delimit a span. // -// Emphasis is matched only where it delimits — at a boundary — so a literal -// underscore inside a word survives. Stripping every `_` turned "Handle user_id" -// into "Handle user id", which is a different identifier. -var markup = regexp.MustCompile(`!\[[^\]]*\]\([^)]*\)|<[^>]+>|(^|\s)[*_` + "`" + `#]+|[*_` + "`" + `#]+(\s|$)`) +// Both halves are deliberately narrow. A generic <[^>]+> would eat `Map` from a title, and stripping every # would turn "#123" into "123", so +// only known tags and boundary emphasis go. Heading markers are handled by +// headingOf, which is where they mean something. +var markup = regexp.MustCompile(`!\[[^\]]*\]\([^)]*\)` + + `|]*)?/?>` + + "|(^|\\s)[*_`]+|[*_`]+(\\s|$)") // rubric matches CodeRabbit's fixed severity header — "🎯 Functional // Correctness | 🟡 Minor | ⚡ Quick win" — which is the same on every finding @@ -21,10 +23,16 @@ var markup = regexp.MustCompile(`!\[[^\]]*\]\([^)]*\)|<[^>]+>|(^|\s)[*_` + "`" + // become "B configuration". var rubric = regexp.MustCompile(`^[^|]*\b(Correctness|Maintainability|Security|Performance|Reliability|Quality)\b[^|]*\|[^|]*\|[^|]*\|?\s*`) -// severityOnly matches a title that is just a severity label — "High Severity", -// "🟠 High", "Critical" — which is what the first bold span is for Bugbot and -// Macroscope. It carries no information a severity field does not already hold. -var severityOnly = regexp.MustCompile(`(?i)^\W*(critical|high|medium|low|major|minor|blocker|trivial|info)(\s+severity)?\W*$`) +// severityWord is the vocabulary Bugbot and Macroscope lead with. +const severityWord = `critical|high|medium|low|major|minor|blocker|trivial|info` + +// severityOnly matches a title that is nothing but a severity label — "High +// Severity", "Critical" — which carries nothing a severity field does not. +var severityOnly = regexp.MustCompile(`(?i)^\W*(` + severityWord + `)(\s+severity)?\W*$`) + +// severityPrefix matches a leading severity label, which Macroscope puts in +// front of the path on its first line. +var severityPrefix = regexp.MustCompile(`(?i)^\W*(` + severityWord + `)(\s+severity)?\b[\s:—-]*`) // ThreadTitle reduces a review comment to one readable line, per bot. // @@ -35,24 +43,31 @@ var severityOnly = regexp.MustCompile(`(?i)^\W*(critical|high|medium|low|major|m // Severity" for the others. // // It lives here because this is where bot wording lives; a listing in the -// orchestration layer must not have to know how a bot writes a heading. -func ThreadTitle(author, body string) string { - candidates := []string{TitleFromDetailedBlock(body)} - if co, ok := CoReviewerByName(author); ok && co.Name != "" { - // A heading, where the bots that lead with a severity label put the - // actual summary. - candidates = append([]string{headingOf(body)}, candidates...) +// orchestration layer must not have to know how a bot writes a heading. Whether +// the author IS a reviewer is the caller's question, not this package's: it +// depends on the configured primary and on matching logins rather than config +// names, and a person whose handle happens to be "codex" is not the bot. +func ThreadTitle(bot bool, body string) string { + var candidates []string + if bot { + // A heading first, for the bots that lead with a severity label and put + // the summary in one; then the bold span, then the first prose line, + // which is where Macroscope's summary lives. + candidates = append(candidates, headingOf(body), TitleFromDetailedBlock(body), firstProse(body)) + } else { + // A person's first line is what they meant to say. Preferring a bold + // span picks up a label they wrote later — "Suggestion:" instead of the + // question they asked. + candidates = append(candidates, firstProse(body), TitleOf(body)) } - candidates = append(candidates, TitleOf(body)) for _, candidate := range candidates { - title := cleanTitle(candidate) - if title != "" && !severityOnly.MatchString(title) { + if title := usableTitle(candidate); title != "" { return title } } - // Everything looked like a severity label; the least bad answer is the - // first non-empty one rather than nothing at all. + // Everything reduced to a severity label or a path; the least bad answer is + // the first non-empty candidate rather than nothing at all. for _, candidate := range candidates { if title := cleanTitle(candidate); title != "" { return title @@ -61,6 +76,48 @@ func ThreadTitle(author, body string) string { return "" } +// usableTitle is a cleaned candidate that actually says something: not a bare +// severity label, and not just the file it points at. +func usableTitle(candidate string) string { + title := cleanTitle(candidate) + if title == "" || severityOnly.MatchString(title) { + return "" + } + // A line that is only the rubric describes every finding equally, so it + // describes none of them. + if rubric.MatchString(candidate) && strings.TrimSpace(rubric.ReplaceAllString(cleanTitle(candidate), "")) == "" { + return "" + } + if rest := strings.TrimSpace(severityPrefix.ReplaceAllString(title, "")); rest == "" || pathLike(rest) { + return "" + } + return title +} + +// pathLike reports whether text is a bare file reference, which is a location +// rather than a description of what is wrong. +func pathLike(text string) bool { + if strings.ContainsAny(text, " \t") { + return false + } + return strings.ContainsAny(text, "/.:") +} + +// firstProse returns the first line that reads like a sentence rather than a +// severity label or a path. Macroscope's summary is its third line. +func firstProse(body string) string { + for _, line := range strings.Split(body, "\n") { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "<") || strings.HasPrefix(line, "#") { + continue + } + if usableTitle(line) != "" { + return line + } + } + return "" +} + // headingOf returns the first Markdown heading's text. func headingOf(body string) string { for _, line := range strings.Split(body, "\n") { @@ -72,6 +129,9 @@ func headingOf(body string) string { } func cleanTitle(title string) string { + // TitleOf caps at 180 BYTES, which can leave a half-written rune; JSON then + // renders it as a replacement character. + title = strings.ToValidUTF8(title, "") title = strings.Join(strings.Fields(markup.ReplaceAllString(title, " ")), " ") if trimmed := strings.TrimSpace(rubric.ReplaceAllString(title, "")); trimmed != "" { title = trimmed diff --git a/internal/dialect/title_test.go b/internal/dialect/title_test.go index 95bedadb..addb203a 100644 --- a/internal/dialect/title_test.go +++ b/internal/dialect/title_test.go @@ -1,6 +1,8 @@ package dialect import ( + "os" + "path/filepath" "strings" "testing" "unicode/utf8" @@ -40,20 +42,20 @@ func TestThreadTitleReadsAsText(t *testing.T) { }, } { t.Run(tc.name, func(t *testing.T) { - if got := ThreadTitle("coderabbitai[bot]", tc.body); got != tc.want { + if got := ThreadTitle(true, tc.body); got != tc.want { t.Errorf("threadTitle = %q, want %q", got, tc.want) } }) } - long := ThreadTitle("coderabbitai[bot]", strings.Repeat("x", 400)) + long := ThreadTitle(true, strings.Repeat("x", 400)) if len([]rune(long)) > 120 { t.Errorf("title is %d chars; a listing line must stay short", len([]rune(long))) } // Truncation cuts runes, not bytes: splitting a multi-byte character leaves // invalid UTF-8, which JSON renders as a replacement character. - wide := ThreadTitle("coderabbitai[bot]", strings.Repeat("é", 400)) + wide := ThreadTitle(true, strings.Repeat("é", 400)) if !utf8.ValidString(wide) { t.Errorf("truncated title is not valid UTF-8: %q", wide) } @@ -83,9 +85,57 @@ func TestThreadTitleSkipsASeverityLabel(t *testing.T) { }, } { t.Run(tc.name, func(t *testing.T) { - if got := ThreadTitle(tc.author, tc.body); got != tc.want { + if got := ThreadTitle(true, tc.body); got != tc.want { t.Errorf("ThreadTitle = %q, want %q", got, tc.want) } }) } } + +// Against the real captured comments, which is what the corpus is for. Each of +// these formats produced a title that said nothing: "High Severity" for Bugbot, +// the severity-and-path line for Macroscope. +func TestThreadTitleAgainstTheCorpus(t *testing.T) { + for _, tc := range []struct{ file, want string }{ + {"bugbot/inline-finding-high.md", "Queue cap desyncs stores"}, + {"bugbot/inline-finding-medium.md", "Composer acks unrelated queue changes"}, + {"macroscope/inline-finding-high.md", "`writeScreenshotFile` writes directly to `absolutePath`"}, + } { + t.Run(tc.file, func(t *testing.T) { + body, err := os.ReadFile(filepath.Join("testdata", tc.file)) + if err != nil { + t.Skipf("corpus file missing: %v", err) + } + got := ThreadTitle(true, string(body)) + if !strings.HasPrefix(got, strings.TrimPrefix(tc.want, "`")) && + !strings.Contains(got, strings.Trim(strings.SplitN(tc.want, " ", 2)[0], "`")) { + t.Errorf("ThreadTitle = %q, want it to start from %q", got, tc.want) + } + if severityOnly.MatchString(got) { + t.Errorf("ThreadTitle = %q — a severity label is not a summary", got) + } + }) + } +} + +// Titles carry code and issue references. A generic tag or heading strip eats +// them: `Map` becomes `Map`, and `#123` becomes `123`. +func TestThreadTitleKeepsCodeAndReferences(t *testing.T) { + for _, tc := range []struct{ body, want string }{ + {"**Handle Map correctly**\n\ndetail", "Handle Map correctly"}, + {"**Fix the leak from #123**\n\ndetail", "Fix the leak from #123"}, + } { + if got := ThreadTitle(true, tc.body); got != tc.want { + t.Errorf("ThreadTitle = %q, want %q", got, tc.want) + } + } +} + +// A person's first line is what they meant to say; a label they bolded later is +// not a better summary of it. +func TestThreadTitlePrefersAHumansFirstLine(t *testing.T) { + body := "Could we simplify this?\n\n**Suggestion:** use a map.\n" + if got := ThreadTitle(false, body); got != "Could we simplify this?" { + t.Errorf("ThreadTitle = %q, want the question they asked", got) + } +} From b243465a7601277fc9d641a5d13f196ebda65ff0 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 22:50:07 +0200 Subject: [PATCH 5/7] Say nothing rather than say the severity twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A comment whose every candidate reduced to a severity label or a bare path still got a title: the first non-empty candidate. That put "High Severity" back in the listing — exactly the label the extraction exists to reject — and for Macroscope it repeated the path the thread already carries. Empty is the honest answer. Title is omitempty, so the entry falls back to path, line and URL, which say strictly more. --- internal/dialect/title.go | 13 +++++-------- internal/dialect/title_test.go | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/internal/dialect/title.go b/internal/dialect/title.go index 00a6b441..1983e4a8 100644 --- a/internal/dialect/title.go +++ b/internal/dialect/title.go @@ -40,7 +40,8 @@ var severityPrefix = regexp.MustCompile(`(?i)^\W*(` + severityWord + `)(\s+sever // rubric, Codex leads with a badge image, Bugbot opens with a `###` heading // followed by a bold severity, and Macroscope starts with an emoji severity and // the path. Taking the first bold span works for two of them and reports "High -// Severity" for the others. +// Severity" for the others. It returns "" when nothing in the comment describes +// the finding, since a non-descriptive title is worse than none. // // It lives here because this is where bot wording lives; a listing in the // orchestration layer must not have to know how a bot writes a heading. Whether @@ -66,13 +67,9 @@ func ThreadTitle(bot bool, body string) string { return title } } - // Everything reduced to a severity label or a path; the least bad answer is - // the first non-empty candidate rather than nothing at all. - for _, candidate := range candidates { - if title := cleanTitle(candidate); title != "" { - return title - } - } + // Everything reduced to a severity label or a path. Empty is the honest + // answer: the listing omits the title and still shows path, line and URL, + // which is strictly more than "High Severity" would have said. return "" } diff --git a/internal/dialect/title_test.go b/internal/dialect/title_test.go index addb203a..db9b5dd4 100644 --- a/internal/dialect/title_test.go +++ b/internal/dialect/title_test.go @@ -83,6 +83,20 @@ func TestThreadTitleSkipsASeverityLabel(t *testing.T) { "🟠 **High** internal/crq/service.go\n\n### Slot released twice\n\nDetail.", "Slot released twice", }, + { + // Nothing left to say: no title beats one that repeats the severity + // field. The listing still carries path, line and URL. + "nothing but the severity", + "cursor[bot]", + "**High Severity**\n", + "", + }, + { + "nothing but the severity and the path", + "macroscopeapp[bot]", + "🟠 **High** internal/crq/service.go\n", + "", + }, } { t.Run(tc.name, func(t *testing.T) { if got := ThreadTitle(true, tc.body); got != tc.want { From 3e0c37c6238118ae4e4fd246f5a1376daaf2d03d Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 00:35:17 +0200 Subject: [PATCH 6/7] Let a listing agree with feedback on who is a bot CRQ_FEEDBACK_BOTS can name a reviewer that is neither the primary nor a registered co-reviewer. `crq feedback` surfaces its findings, but the thread listing called it a person: it read as "author", and its title was parsed with the human heuristic, which prefers the first line and so reports the severity label a bot puts there. The markup stripping had a second way to be wrong about a title: a boundary run mixed backticks with underscores, so `__init__` in a title lost its underscores along with the code span and the listing claimed the finding was about "init". A run is now one delimiter kind. --- internal/crq/threads.go | 11 ++++++++++- internal/crq/threads_test.go | 36 ++++++++++++++++++++++++++++++++++ internal/dialect/title.go | 6 +++++- internal/dialect/title_test.go | 8 ++++++++ 4 files changed, 59 insertions(+), 2 deletions(-) diff --git a/internal/crq/threads.go b/internal/crq/threads.go index e38e0d1f..86520c71 100644 --- a/internal/crq/threads.go +++ b/internal/crq/threads.go @@ -86,12 +86,21 @@ func (s *Service) OpenThreads(ctx context.Context, repo string, pr int) ([]OpenT } // isReviewerBot reports whether login is a reviewer crq knows: the configured -// primary, a configured co-reviewer, or a registry entry. +// primary, a configured feedback bot, a configured co-reviewer, or a registry +// entry. func isReviewerBot(cfg Config, login string) bool { key := dialect.NormalizeBotName(login) if key == dialect.NormalizeBotName(cfg.Bot) { return true } + // Whatever CRQ_FEEDBACK_BOTS names is a bot to `crq feedback`, so it has to + // be one here too — otherwise the same thread is a bot's finding in one + // command and a human's comment in the other. + for _, bot := range cfg.FeedbackBots { + if dialect.NormalizeBotName(bot) == key { + return true + } + } for _, cb := range cfg.CoBots { if dialect.NormalizeBotName(cb.Login) == key { return true diff --git a/internal/crq/threads_test.go b/internal/crq/threads_test.go index 998155f7..93bb90a6 100644 --- a/internal/crq/threads_test.go +++ b/internal/crq/threads_test.go @@ -53,6 +53,42 @@ func TestOpenThreadsIncludesOutdatedAndOrdersThem(t *testing.T) { } } +// A login in CRQ_FEEDBACK_BOTS is a bot to `crq feedback`, which surfaces its +// findings. Listing the same thread as a human's would disagree with the command +// an agent reads next, and would parse the title with the human heuristic. +func TestOpenThreadsNamesAConfiguredFeedbackBot(t *testing.T) { + gh := newFakeGitHub() + gh.graphQL = func(query string, _ map[string]any, out any) error { + if !strings.Contains(query, "reviewThreads") { + return nil + } + return json.Unmarshal([]byte(`{"repository":{"pullRequest":{"reviewThreads":{ + "pageInfo":{"hasNextPage":false,"endCursor":""}, + "nodes":[{"id":"T_custom","isResolved":false,"isOutdated":false,"path":"a.go","line":1, + "comments":{"totalCount":1,"nodes":[{"body":"### Nil deref\n\n**High Severity**\n\nDetail.", + "author":{"login":"reviewdog[bot]"}}]}}] + }}}}`), out) + } + cfg := firingConfig() + cfg.FeedbackBots = append(cfg.FeedbackBots, "reviewdog[bot]") + svc := NewService(cfg, gh, NewMemoryStore(cfg), nil) + + threads, err := svc.OpenThreads(context.Background(), "o/r", 1) + if err != nil { + t.Fatal(err) + } + if len(threads) != 1 { + t.Fatalf("got %d threads, want the custom reviewer's", len(threads)) + } + if threads[0].Bot != "reviewdog" || threads[0].Author != "" { + t.Errorf("thread = %+v, want the configured feedback bot named as a bot", threads[0]) + } + // Read as a bot's: the heading is the summary and the bold span is severity. + if threads[0].Title != "Nil deref" { + t.Errorf("title = %q, want the bot's heading rather than its severity", threads[0].Title) + } +} + // The command promises every unresolved thread, so it also gets humans'. Copying // the author into `bot` made the output say `"bot":"alice"`. func TestOpenThreadsDoesNotCallAPersonABot(t *testing.T) { diff --git a/internal/dialect/title.go b/internal/dialect/title.go index 1983e4a8..40407e60 100644 --- a/internal/dialect/title.go +++ b/internal/dialect/title.go @@ -12,9 +12,13 @@ import ( // User>` from a title, and stripping every # would turn "#123" into "123", so // only known tags and boundary emphasis go. Heading markers are handled by // headingOf, which is where they mean something. +// +// A boundary run is one delimiter KIND, never a mixture: in "`__init__`" the +// run is the backtick alone, so the underscores stay part of the identifier. +// Merging them would report the finding as being about "init". var markup = regexp.MustCompile(`!\[[^\]]*\]\([^)]*\)` + `|]*)?/?>` + - "|(^|\\s)[*_`]+|[*_`]+(\\s|$)") + "|(^|\\s)(?:_+|[*`]+)|(?:_+|[*`]+)(\\s|$)") // rubric matches CodeRabbit's fixed severity header — "🎯 Functional // Correctness | 🟡 Minor | ⚡ Quick win" — which is the same on every finding diff --git a/internal/dialect/title_test.go b/internal/dialect/title_test.go index db9b5dd4..725fb035 100644 --- a/internal/dialect/title_test.go +++ b/internal/dialect/title_test.go @@ -40,6 +40,14 @@ func TestThreadTitleReadsAsText(t *testing.T) { "**Handle user_id and api_key**\n\nDetail.", "Handle user_id and api_key", }, + { + // The code span goes, the identifier stays whole: a run of markup is + // one delimiter kind, so the backtick does not carry the dunder with + // it and leave the title talking about "init". + "identifier with leading underscores in a code span", + "**Preserve `__init__` ordering**\n\nDetail.", + "Preserve __init__ ordering", + }, } { t.Run(tc.name, func(t *testing.T) { if got := ThreadTitle(true, tc.body); got != tc.want { From e169b677e2c20fd310eba617d5b2fdf6390133b5 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 10:24:30 +0200 Subject: [PATCH 7/7] Tighten review title corpus coverage --- internal/dialect/title.go | 28 +++++++++++++++++++++++++--- internal/dialect/title_test.go | 10 +++++----- 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/internal/dialect/title.go b/internal/dialect/title.go index 40407e60..b33a6d36 100644 --- a/internal/dialect/title.go +++ b/internal/dialect/title.go @@ -109,7 +109,8 @@ func pathLike(text string) bool { func firstProse(body string) string { for _, line := range strings.Split(body, "\n") { line = strings.TrimSpace(line) - if line == "" || strings.HasPrefix(line, "<") || strings.HasPrefix(line, "#") { + _, heading := headingText(line) + if line == "" || strings.HasPrefix(line, "<") || heading { continue } if usableTitle(line) != "" { @@ -122,13 +123,34 @@ func firstProse(body string) string { // headingOf returns the first Markdown heading's text. func headingOf(body string) string { for _, line := range strings.Split(body, "\n") { - if trimmed := strings.TrimSpace(line); strings.HasPrefix(trimmed, "#") { - return strings.TrimSpace(strings.TrimLeft(trimmed, "# ")) + if heading, ok := headingText(line); ok { + return heading } } return "" } +// headingText recognizes an ATX heading marker only when its hashes are +// followed by whitespace or the end of the line. An issue reference such as +// "#123 breaks parsing" is prose and must keep its hash. +func headingText(line string) (string, bool) { + line = strings.TrimSpace(line) + hashes := 0 + for hashes < len(line) && hashes < 6 && line[hashes] == '#' { + hashes++ + } + if hashes == 0 || (hashes < len(line) && line[hashes] == '#') { + return "", false + } + if hashes == len(line) { + return "", true + } + if line[hashes] != ' ' && line[hashes] != '\t' { + return "", false + } + return strings.TrimSpace(line[hashes:]), true +} + func cleanTitle(title string) string { // TitleOf caps at 180 BYTES, which can leave a half-written rune; JSON then // renders it as a replacement character. diff --git a/internal/dialect/title_test.go b/internal/dialect/title_test.go index 725fb035..18e7f4f3 100644 --- a/internal/dialect/title_test.go +++ b/internal/dialect/title_test.go @@ -121,17 +121,16 @@ func TestThreadTitleAgainstTheCorpus(t *testing.T) { for _, tc := range []struct{ file, want string }{ {"bugbot/inline-finding-high.md", "Queue cap desyncs stores"}, {"bugbot/inline-finding-medium.md", "Composer acks unrelated queue changes"}, - {"macroscope/inline-finding-high.md", "`writeScreenshotFile` writes directly to `absolutePath`"}, + {"macroscope/inline-finding-high.md", "writeScreenshotFile writes directly to absolutePath after WorkspacePaths.resolveRelativePathWithinRoot validates it, ..."}, } { t.Run(tc.file, func(t *testing.T) { body, err := os.ReadFile(filepath.Join("testdata", tc.file)) if err != nil { - t.Skipf("corpus file missing: %v", err) + t.Fatalf("read corpus fixture: %v", err) } got := ThreadTitle(true, string(body)) - if !strings.HasPrefix(got, strings.TrimPrefix(tc.want, "`")) && - !strings.Contains(got, strings.Trim(strings.SplitN(tc.want, " ", 2)[0], "`")) { - t.Errorf("ThreadTitle = %q, want it to start from %q", got, tc.want) + if got != tc.want { + t.Errorf("ThreadTitle = %q, want %q", got, tc.want) } if severityOnly.MatchString(got) { t.Errorf("ThreadTitle = %q — a severity label is not a summary", got) @@ -146,6 +145,7 @@ func TestThreadTitleKeepsCodeAndReferences(t *testing.T) { for _, tc := range []struct{ body, want string }{ {"**Handle Map correctly**\n\ndetail", "Handle Map correctly"}, {"**Fix the leak from #123**\n\ndetail", "Fix the leak from #123"}, + {"#123 breaks parsing\n\ndetail", "#123 breaks parsing"}, } { if got := ThreadTitle(true, tc.body); got != tc.want { t.Errorf("ThreadTitle = %q, want %q", got, tc.want)