diff --git a/README.md b/README.md index fee847b7..5bf6c377 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 dismiss [...] --reason "" # account for a finding with no thread diff --git a/cmd/crq/main.go b/cmd/crq/main.go index 494bbdde..4f4c9278 100644 --- a/cmd/crq/main.go +++ b/cmd/crq/main.go @@ -211,6 +211,23 @@ 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 + } + if err := cfg.RequireState(); err != nil { + fatal(err) + 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) == "" { @@ -385,6 +402,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 dismiss [...] --reason "" account for a finding GitHub gives you no thread to close crq autoreview [--once] [--no-incremental] @@ -540,6 +558,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 "dismiss": fmt.Print(`crq dismiss [...] --reason "" diff --git a/internal/crq/threads.go b/internal/crq/threads.go new file mode 100644 index 00000000..86520c71 --- /dev/null +++ b/internal/crq/threads.go @@ -0,0 +1,115 @@ +package crq + +import ( + "context" + "sort" + + "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"` + // 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. + 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] + // 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. + isBot := isReviewerBot(s.cfg, first.Author.Login) + if isBot { + open.Bot = dialect.NormalizeBotName(first.Author.Login) + } else { + open.Author = first.Author.Login + } + open.Title = dialect.ThreadTitle(isBot, first.Body) + open.URL = first.URL + if open.Path == "" { + open.Path = first.Path + } + if open.Line == 0 { + // 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) + } + // 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 +} + +// isReviewerBot reports whether login is a reviewer crq knows: the configured +// 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 + } + } + // 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/crq/threads_test.go b/internal/crq/threads_test.go new file mode 100644 index 00000000..93bb90a6 --- /dev/null +++ b/internal/crq/threads_test.go @@ -0,0 +1,129 @@ +package crq + +import ( + "context" + "encoding/json" + "strings" + "testing" +) + +// 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]) + } + // 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]) + } +} + +// 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) { + 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..b33a6d36 --- /dev/null +++ b/internal/dialect/title.go @@ -0,0 +1,168 @@ +package dialect + +import ( + "regexp" + "strings" +) + +// 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. +// +// 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. +// +// 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|$)") + +// 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*`) + +// 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. +// +// 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 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 +// 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)) + } + + for _, candidate := range candidates { + if title := usableTitle(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 "" +} + +// 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) + _, heading := headingText(line) + if line == "" || strings.HasPrefix(line, "<") || heading { + 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") { + 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. + title = strings.ToValidUTF8(title, "") + 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..18e7f4f3 --- /dev/null +++ b/internal/dialect/title_test.go @@ -0,0 +1,163 @@ +package dialect + +import ( + "os" + "path/filepath" + "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", + }, + { + // 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 { + t.Errorf("threadTitle = %q, want %q", got, tc.want) + } + }) + } + + 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(true, 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", + }, + { + // 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 { + 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 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.Fatalf("read corpus fixture: %v", err) + } + got := ThreadTitle(true, string(body)) + 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) + } + }) + } +} + +// 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"}, + {"#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) + } + } +} + +// 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) + } +} diff --git a/llms.txt b/llms.txt index 018a014c..a47d3cd3 100644 --- a/llms.txt +++ b/llms.txt @@ -188,6 +188,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 39b9f7e8..2efe0e86 100644 --- a/skills/coderabbit-queue/SKILL.md +++ b/skills/coderabbit-queue/SKILL.md @@ -160,6 +160,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