From ad7a9b2d5888d88f64c54a365151f69074019efb Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 17:51:32 +0200 Subject: [PATCH 1/2] Let crq find the PR, and say where it stands in one line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things an agent had to work around by hand. A bare `crq next` inside a checkout now infers its target: read the branch, match a remote to a repository slug, and ask GitHub which open pull request has that head. The skill has told agents to run crq from inside the checkout since `local_work` landed; now that being there is worth something, they no longer have to paste a repo and a number they already stand in. And `crq status --line` renders the queue as a single line for a status bar, so "still going?" has an answer that costs no turn. It reports only what crq can know: stranded outranks blocked outranks reviewing, and `next #N` appears only when the queue actually knows who is next. `crq status` now rejects a positional argument instead of ignoring it โ€” `crq status 41` looked like it reported on that PR. --- cmd/crq/main.go | 73 +++++++++++++++++++++++++------- internal/crq/service.go | 4 ++ internal/crq/service_test.go | 29 +++++++++++++ internal/crq/state.go | 6 +++ internal/crq/target.go | 73 ++++++++++++++++++++++++++++++++ internal/crq/target_test.go | 46 ++++++++++++++++++++ internal/state/dashboard_test.go | 60 ++++++++++++++++++++++++++ internal/state/statusline.go | 50 ++++++++++++++++++++++ llms.txt | 1 + skills/coderabbit-queue/SKILL.md | 1 + 10 files changed, 327 insertions(+), 16 deletions(-) create mode 100644 internal/crq/target.go create mode 100644 internal/crq/target_test.go create mode 100644 internal/state/statusline.go diff --git a/cmd/crq/main.go b/cmd/crq/main.go index d6816d4..caf6f71 100644 --- a/cmd/crq/main.go +++ b/cmd/crq/main.go @@ -93,15 +93,29 @@ func run(ctx context.Context, args []string) int { fmt.Printf("export CRQ_STATE_REF=%q\n", result.StateRef) return 0 case "status": + if bad, found := unknownFlag(args[1:], "--line"); found { + fatal(fmt.Errorf("unknown flag %s (usage: crq status [--line])", bad)) + return 1 + } + // status takes no target: silently ignoring one would let `crq status 41` + // look like it reported on that PR. + if extra := positional(args[1:]); len(extra) > 0 { + fatal(fmt.Errorf("crq status takes no arguments, got %q (usage: crq status [--line])", extra[0])) + return 1 + } if err := cfg.RequireState(); err != nil { fatal(err) return 1 } - _, dashboard, err := service.Status(ctx) + state, dashboard, err := service.Status(ctx) if err != nil { fatal(err) return 1 } + if hasFlag(args[1:], "--line") { + fmt.Println(crq.StatusLine(state, cfg)) + return 0 + } fmt.Print(dashboard) return 0 case "feedback": @@ -122,17 +136,16 @@ func run(ctx context.Context, args []string) int { fatal(fmt.Errorf("unknown flag %s (usage: crq next [--wait])", bad)) return 1 } - repo, pr, ok := repoPR(positional(args[1:])) - if !ok { - fatal(errors.New("usage: crq next [--wait]")) + if err := cfg.RequireState(); err != nil { + fatal(err) return 1 } - if err := cfg.RequireState(); err != nil { + repo, pr, err := target(ctx, service, positional(args[1:]), "crq next [ ] [--wait]") + if err != nil { fatal(err) return 1 } var report crq.NextReport - var err error if hasFlag(args[1:], "--wait") { report, err = service.NextWaiting(ctx, repo, pr) } else { @@ -147,12 +160,12 @@ func run(ctx context.Context, args []string) int { // caller never has to interpret two things at once. return 0 case "wait": - repo, pr, ok := repoPR(args[1:]) - if !ok { - fatal(errors.New("usage: crq wait ")) + if err := cfg.RequireState(); err != nil { + fatal(err) return 1 } - if err := cfg.RequireState(); err != nil { + repo, pr, err := target(ctx, service, args[1:], "crq wait [ ]") + if err != nil { fatal(err) return 1 } @@ -309,11 +322,11 @@ func usage() { fmt.Print(`crq - CodeRabbit review queue for humans and automation QUEUE WORKFLOWS - crq next ask what to do next about a PR (the agent loop) - crq wait block until there IS something to do, then say what + crq next [ ] ask what to do next about a PR (the agent loop) + crq wait [ ] block until there IS something to do, then say what crq loop queue one PR review round, then emit JSON feedback crq autoreview keep open PRs reviewed through the same queue - crq status show the queue, in-flight review, and quota state + crq status [--line] show the queue, in-flight review, and quota state DRIVING A PR REVIEW Call crq next, do exactly what .action says, call it again. That is the whole loop. @@ -332,7 +345,9 @@ DRIVING A PR REVIEW USAGE crq init initialize state in CRQ_REPO - crq next [--wait] emit the single next action as JSON (--wait blocks) + crq next [ ] [--wait] emit the single next action as JSON (--wait blocks) + omit the target inside a checkout: crq reads the + remote and branch to find the pull request crq wait block until actionable, then emit that action as JSON crq loop coordinated trigger -> wait -> JSON feedback/convergence crq feedback emit normalized actionable review findings as JSON @@ -346,7 +361,7 @@ USAGE crq preflight [--type all|committed|uncommitted] [--base ] local CodeRabbit CLI pre-push review as JSON crq doctor emit JSON readiness report for agents and humans - crq status print the dashboard + crq status [--line] print the dashboard, or one line for a status bar crq cancel remove queued/in-flight state for a PR crq debug maintenance tools; not for normal review loops @@ -560,7 +575,18 @@ Checks include: Use this before a human-run loop, background watcher, or autonomous agent. `) case "status": - fmt.Print("crq status\n\nPrint the dashboard rendered from the CAS state ref.\n") + fmt.Print(`crq status [--line] + +Print the dashboard rendered from the CAS state ref. + +--line prints ONE line instead, for a harness status bar: + + crq ๐Ÿ”ฌ #41 reviewing ยท 2 queued + +That answers "is it still going?" continuously, which is otherwise the question a +session spends the most tool calls on. It names the next PR only when the queue +actually knows which one that is. +`) case "cancel": fmt.Print("crq cancel \n\nRemove a PR from queued/in-flight crq state.\n") case "debug": @@ -629,6 +655,21 @@ func hasFlag(args []string, flag string) bool { return false } +// target resolves which PR a command is about: the two arguments if given, and +// otherwise the checkout the caller is standing in. Agents drive the loop from +// inside the repository, so making them carry a repo and number they are already +// standing in is two chances to name the wrong PR silently. +func target(ctx context.Context, service *crq.Service, args []string, usage string) (string, int, error) { + switch repo, pr, ok := repoPR(args); { + case ok: + return repo, pr, nil + case len(args) == 0: + return service.InferTarget(ctx) + default: + return "", 0, fmt.Errorf("usage: %s", usage) + } +} + // unknownFlag returns the first flag in args that is not in allowed. // // positional() drops anything starting with "-", so without this a mistyped diff --git a/internal/crq/service.go b/internal/crq/service.go index 287cebd..ea6bb15 100644 --- a/internal/crq/service.go +++ b/internal/crq/service.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io" + "net/url" "sort" "strconv" "strings" @@ -37,6 +38,9 @@ type GitHubAPI interface { SearchOpenPRs(context.Context, string, bool, int) ([]ghapi.SearchPR, error) EachOpenPR(context.Context, string, bool, func(ghapi.SearchPR) (bool, error)) error GraphQL(context.Context, string, map[string]any, any) error + // ListPulls finds pull requests, filtered by the query. crq uses it to map a + // checkout's branch to the PR it belongs to. + ListPulls(context.Context, string, url.Values) ([]ghapi.Pull, error) // GetRef reads a ref's SHA. It is the cheapest "did anything change?" probe // crq has โ€” a conditional GET that costs no quota while the ref is // unchanged โ€” which is what `crq wait` idles on. diff --git a/internal/crq/service_test.go b/internal/crq/service_test.go index 19c4241..5746224 100644 --- a/internal/crq/service_test.go +++ b/internal/crq/service_test.go @@ -4,6 +4,8 @@ import ( "context" "encoding/json" "errors" + "net/url" + "sort" "strconv" "strings" "sync" @@ -235,6 +237,33 @@ func (f *fakeGitHub) EachOpenPR(_ context.Context, _ string, _ bool, fn func(gha return nil } +// ListPulls answers the branch->PR lookup target inference uses. +// +// It honours the head filter, because a fake that ignores it would let a caller +// asking for the wrong branch pass: every PR in the repository would come back +// and a single-result assertion would still hold. +func (f *fakeGitHub) ListPulls(_ context.Context, repo string, query url.Values) ([]ghapi.Pull, error) { + f.mu.Lock() + defer f.mu.Unlock() + wantRef := "" + if head := query.Get("head"); head != "" { + _, wantRef, _ = strings.Cut(head, ":") // "owner:branch" + } + var out []ghapi.Pull + for key, p := range f.pulls { + if !strings.HasPrefix(key, strings.ToLower(repo)+"#") { + continue + } + if wantRef != "" && p.Head.Ref != wantRef { + continue + } + out = append(out, p) + } + // Deterministic: map iteration order must not decide what a test sees. + sort.Slice(out, func(i, j int) bool { return out[i].Number < out[j].Number }) + return out, nil +} + // GetRef reports the fake state ref. It is what `crq wait` idles on, so tests // count the reads to assert the wait stays cheap. func (f *fakeGitHub) GetRef(_ context.Context, _, _ string) (string, error) { diff --git a/internal/crq/state.go b/internal/crq/state.go index be37562..d6cbfc3 100644 --- a/internal/crq/state.go +++ b/internal/crq/state.go @@ -100,6 +100,12 @@ func renderDashboard(st State, cfg Config) string { func renderTitle(st State, cfg Config) string { return crqstate.RenderTitle(st, cfg.storeConfig()) } + +// StatusLine renders the queue as a single line for a harness status bar. +func StatusLine(st State, cfg Config) string { + return crqstate.StatusLine(st, cfg.storeConfig()) +} + func issueBody(st State, cfg Config) (string, error) { return crqstate.IssueBody(st, cfg.storeConfig()) } diff --git a/internal/crq/target.go b/internal/crq/target.go new file mode 100644 index 0000000..9cb3070 --- /dev/null +++ b/internal/crq/target.go @@ -0,0 +1,73 @@ +package crq + +import ( + "context" + "fmt" + "net/url" + "os/exec" + "strings" +) + +// InferTarget works out which PR the caller means from the checkout they are +// standing in. +// +// Every crq command took explicitly, which is two arguments an agent +// has to carry through a loop it is already driving from inside the repository โ€” +// and getting either wrong is silent, because another repo's PR number is usually +// a valid PR. The checkout already knows: its remote names the repository, and +// its branch names the pull request. +// +// It is only ever a convenience. Anything it cannot establish is an error naming +// what was missing, never a guess. +func (s *Service) InferTarget(ctx context.Context) (string, int, error) { + branch, err := gitIn(ctx, "rev-parse", "--abbrev-ref", "HEAD") + if err != nil { + return "", 0, fmt.Errorf("not inside a git checkout, so cannot be inferred: %w", err) + } + if branch == "HEAD" { + return "", 0, fmt.Errorf("this checkout is detached, so there is no branch to find a pull request for") + } + remotes, err := gitIn(ctx, "remote", "-v") + if err != nil { + return "", 0, fmt.Errorf("could not read this checkout's remotes: %w", err) + } + repo := "" + for _, line := range strings.Split(remotes, "\n") { + fields := strings.Fields(line) + if len(fields) < 2 { + continue + } + if slug := repoSlugFromRemote(fields[1]); slug != "" { + repo = slug + break + } + } + if repo == "" { + return "", 0, fmt.Errorf("no github remote found in this checkout") + } + + // Ask for this branch's PR specifically rather than listing them all: the + // head filter is one request whatever the repository's size. + query := url.Values{} + query.Set("state", "open") + query.Set("head", ownerOf(repo)+":"+branch) + pulls, err := s.gh.ListPulls(ctx, repo, query) + if err != nil { + return "", 0, err + } + if len(pulls) == 0 { + return "", 0, fmt.Errorf("no open pull request for %s on branch %s", repo, branch) + } + if len(pulls) > 1 { + return "", 0, fmt.Errorf("%d open pull requests for %s on branch %s; name the one you mean", len(pulls), repo, branch) + } + return repo, pulls[0].Number, nil +} + +func gitIn(ctx context.Context, args ...string) (string, error) { + out, err := exec.CommandContext(ctx, "git", args...).Output() + if err != nil { + return "", err + } + return strings.TrimSpace(string(out)), nil +} diff --git a/internal/crq/target_test.go b/internal/crq/target_test.go new file mode 100644 index 0000000..9e1b531 --- /dev/null +++ b/internal/crq/target_test.go @@ -0,0 +1,46 @@ +package crq + +import ( + "context" + "net/url" + "testing" + + ghapi "github.com/kristofferR/coderabbit-queue/internal/gh" +) + +// Inference must ask for THIS branch's pull request, not take whatever the +// repository happens to return first. A fake that ignored the head filter would +// have hidden the difference, so this pins the query itself. +func TestInferTargetAsksForTheBranchesPull(t *testing.T) { + gh := newFakeGitHub() + for _, tc := range []struct { + pr int + branch string + }{{11, "other-work"}, {12, "the-branch"}, {13, "unrelated"}} { + var p ghapi.Pull + p.State = "open" + p.Number = tc.pr + p.Head.SHA = "aaaaaaaaa" + p.Head.Ref = tc.branch + gh.pulls[fakeKey("owner/repo", tc.pr)] = p + } + + query := url.Values{} + query.Set("head", "owner:the-branch") + got, err := gh.ListPulls(context.Background(), "owner/repo", query) + if err != nil { + t.Fatal(err) + } + if len(got) != 1 || got[0].Number != 12 { + t.Fatalf("head filter returned %+v, want only PR 12 โ€” an unfiltered answer would let a wrong-branch lookup pass", got) + } + + // No filter still returns everything, deterministically ordered. + all, err := gh.ListPulls(context.Background(), "owner/repo", nil) + if err != nil { + t.Fatal(err) + } + if len(all) != 3 || all[0].Number != 11 || all[2].Number != 13 { + t.Fatalf("unfiltered = %+v, want 11,12,13 in order", all) + } +} diff --git a/internal/state/dashboard_test.go b/internal/state/dashboard_test.go index 3327517..40b4b01 100644 --- a/internal/state/dashboard_test.go +++ b/internal/state/dashboard_test.go @@ -848,3 +848,63 @@ func TestHeldSlotStopsFreeRunningRoundsToo(t *testing.T) { t.Error("nothing may be numbered while the slot is held") } } + +// The status line answers "is it still going?" continuously, so a session never +// has to spend a tool call and a paragraph on it. Each state must read at a +// glance, and it must never claim a next PR the queue itself will not name. +func TestStatusLine(t *testing.T) { + now := time.Now().UTC() + + if got := StatusLine(New(), StoreConfig{}); !strings.Contains(got, "idle") { + t.Errorf("empty state = %q, want idle", got) + } + + ready := stateWith(queuedRound("kristofferr/a", 7, 1, now)) + if got := StatusLine(ready, StoreConfig{}); !strings.Contains(got, "next #7") { + t.Errorf("a ready queue should name what is next, got %q", got) + } + + // Blocked: the countdown is the useful part. + blockedState := stateWith(queuedRound("kristofferr/a", 7, 1, now)) + until := now.Add(30 * time.Minute) + blockedState.Account.BlockedUntil = &until + got := StatusLine(blockedState, StoreConfig{}) + if !strings.Contains(got, "blocked") { + t.Errorf("blocked state = %q, want the block named", got) + } + if strings.Contains(got, "next #") { + t.Errorf("nothing can fire while blocked, so no next may be claimed: %q", got) + } + + // Stranded outranks everything, as on the dashboard. + strandedState := stateWith(queuedRound("kristofferr/a", 7, 1, now)) + reserved := now.Add(-time.Minute) + strandedState.Rounds["kristofferr/a#7"] = func() Round { + r := strandedState.Rounds["kristofferr/a#7"] + r.Phase, r.ReservedAt = PhaseReserved, &reserved + return r + }() + stranded := StatusLine(strandedState, StoreConfig{}) + if !strings.Contains(stranded, "stranded") { + t.Errorf("stranded state = %q, want it named first", stranded) + } + // Precedence, not just presence: a stranded round outranks every state that + // clears by itself, so none of those may share the line. + for _, lower := range []string{"blocked", "idle", "next #", "reviewing"} { + if strings.Contains(stranded, lower) { + t.Errorf("stranded line %q must not also report %q", stranded, lower) + } + } + + // It is a status LINE: anything multi-line corrupts the bar it renders into. + for _, line := range []string{ + StatusLine(New(), StoreConfig{}), + StatusLine(ready, StoreConfig{}), + StatusLine(blockedState, StoreConfig{}), + stranded, + } { + if strings.ContainsAny(line, "\r\n") { + t.Errorf("status line contains a newline: %q", line) + } + } +} diff --git a/internal/state/statusline.go b/internal/state/statusline.go new file mode 100644 index 0000000..caf2afb --- /dev/null +++ b/internal/state/statusline.go @@ -0,0 +1,50 @@ +package state + +import ( + "fmt" + "strings" + "time" +) + +// StatusLine renders the queue as one line, for a harness status bar. +// +// It exists so nobody has to ask a chat agent "is it still going?" โ€” the most +// common question in a review session, and the one that cost the most tokens to +// answer, because answering it meant a tool call and a paragraph. A status bar +// answers it continuously for free. +// +// Everything here is already computed for the dashboard; this is a second +// rendering of the same reduced state, not new logic. +func StatusLine(st State, cfg StoreConfig) string { + now := time.Now().UTC() + queue := st.Queue(now, cfg.MinInterval) + inFlight := inFlightRounds(st) + + var parts []string + switch { + case firstStranded(st, inFlight) != nil: + s := firstStranded(st, inFlight) + parts = append(parts, fmt.Sprintf("โš  #%d stranded", s.PR)) + case st.Account.BlockedUntil != nil && st.Account.BlockedUntil.After(now): + parts = append(parts, fmt.Sprintf("โณ blocked %dm", minutesUntil(*st.Account.BlockedUntil, now))) + case len(inFlight) > 0: + parts = append(parts, fmt.Sprintf("๐Ÿ”ฌ #%d reviewing", inFlight[0].PR)) + case len(queue) == 0: + return "โœ… crq idle" + default: + parts = append(parts, "๐ŸŸข ready") + } + + if n := len(queue); n > 0 { + next := "" + // Name the next PR only when the queue knows which it is; see Queue. + if queue[0].ReadyAt.IsZero() && queue[0].Why == "" { + next = fmt.Sprintf(" next #%d", queue[0].PR) + } + parts = append(parts, fmt.Sprintf("%d queued%s", n, next)) + } + if len(inFlight) > 1 { + parts = append(parts, fmt.Sprintf("%d in flight", len(inFlight))) + } + return "crq " + strings.Join(parts, " ยท ") +} diff --git a/llms.txt b/llms.txt index 922b884..f8633f4 100644 --- a/llms.txt +++ b/llms.txt @@ -44,6 +44,7 @@ finished โ€” is a value crq computes. ```bash crq next OWNER/REPO PR_NUMBER +crq next # inside the checkout, the target is inferred ``` | `.action` | what to do | diff --git a/skills/coderabbit-queue/SKILL.md b/skills/coderabbit-queue/SKILL.md index cab6ef8..801dad1 100644 --- a/skills/coderabbit-queue/SKILL.md +++ b/skills/coderabbit-queue/SKILL.md @@ -21,6 +21,7 @@ Do not design one of your own. ```bash crq next "$REPO" "$PR" +crq next # inside the checkout: crq finds the PR from the remote and branch ``` | `.action` | what to do | From 5a3697b6294990a506e8bfbbc6b0311e2d03e7e6 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 18:11:18 +0200 Subject: [PATCH 2/2] Infer for every command, and stop the status line contradicting itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four review findings on this branch, all correct. Inference reached only next and wait, while the PR claimed every command that takes a target. feedback, loop, cancel and debug enqueue now route through the same helper, so a bare call works wherever a target is taken. Fork checkouts could not be inferred at all. Taking the first remote and its own owner asks /repos/me/app/pulls?head=me:branch, but the pull request is filed against upstream โ€” so the one case where carrying the arguments by hand is most annoying reported that no PR exists. Inference now considers every GitHub remote as a base and every remote owner as a head. A single-remote checkout still costs exactly one request. That makes a remote slug an API lookup rather than a comparison, so it now requires a GitHub remote: repoSlugFromRemote is deliberately loose for matching, where a wrong guess merely fails to match, and would have turned /home/me/code/app into a request for code/app. The status line could say "blocked" and then name the next PR in the same breath โ€” but a quota-free round stays actionable precisely because the account window has no authority over it. Ready work now outranks the block. It could also point past a stranded reservation, which reads as though the queue is moving; the earlier precedence test only proved that case with an empty queue behind it. --- cmd/crq/main.go | 30 ++++----- internal/crq/target.go | 101 +++++++++++++++++++++++-------- internal/crq/target_test.go | 37 +++++++++++ internal/state/dashboard_test.go | 30 +++++++++ internal/state/statusline.go | 20 ++++-- llms.txt | 1 + skills/coderabbit-queue/SKILL.md | 1 + 7 files changed, 175 insertions(+), 45 deletions(-) diff --git a/cmd/crq/main.go b/cmd/crq/main.go index caf6f71..a4b2dbf 100644 --- a/cmd/crq/main.go +++ b/cmd/crq/main.go @@ -119,9 +119,9 @@ func run(ctx context.Context, args []string) int { fmt.Print(dashboard) return 0 case "feedback": - repo, pr, ok := repoPR(args[1:]) - if !ok { - fatal(errors.New("usage: crq feedback ")) + repo, pr, err := target(ctx, service, args[1:], "crq feedback [ ]") + if err != nil { + fatal(err) return 1 } report, err := service.Feedback(ctx, repo, pr) @@ -178,9 +178,9 @@ func run(ctx context.Context, args []string) int { // Same contract as next: the action is the answer, the exit code is not. return 0 case "loop": - repo, pr, ok := repoPR(args[1:]) - if !ok { - fatal(errors.New("usage: crq loop ")) + repo, pr, err := target(ctx, service, args[1:], "crq loop [ ]") + if err != nil { + fatal(err) return 1 } if err := cfg.RequireState(); err != nil { @@ -242,9 +242,9 @@ func run(ctx context.Context, args []string) int { } return 0 case "cancel": - repo, pr, ok := repoPR(args[1:]) - if !ok { - fatal(errors.New("usage: crq cancel ")) + repo, pr, err := target(ctx, service, args[1:], "crq cancel [ ]") + if err != nil { + fatal(err) return 1 } if err := cfg.RequireState(); err != nil { @@ -276,9 +276,9 @@ func debug(ctx context.Context, service *crq.Service, store crq.StateStore, cfg } switch args[0] { case "enqueue": - repo, pr, ok := repoPR(args[1:]) - if !ok { - fatal(errors.New("usage: crq debug enqueue ")) + repo, pr, err := target(ctx, service, args[1:], "crq debug enqueue [ ]") + if err != nil { + fatal(err) return 1 } result, err := service.Enqueue(ctx, repo, pr) @@ -349,8 +349,8 @@ USAGE omit the target inside a checkout: crq reads the remote and branch to find the pull request crq wait block until actionable, then emit that action as JSON - crq loop coordinated trigger -> wait -> JSON feedback/convergence - crq feedback emit normalized actionable review findings as JSON + crq loop [ ] coordinated trigger -> wait -> JSON feedback/convergence + crq feedback [ ] emit normalized actionable review findings as JSON crq resolve [...] resolve addressed GitHub review threads crq decline [...] --reason "" [--keep-open] @@ -362,7 +362,7 @@ USAGE local CodeRabbit CLI pre-push review as JSON crq doctor emit JSON readiness report for agents and humans crq status [--line] print the dashboard, or one line for a status bar - crq cancel remove queued/in-flight state for a PR + crq cancel [ ] remove queued/in-flight state for a PR crq debug maintenance tools; not for normal review loops diff --git a/internal/crq/target.go b/internal/crq/target.go index 9cb3070..36ed7d3 100644 --- a/internal/crq/target.go +++ b/internal/crq/target.go @@ -31,37 +31,90 @@ func (s *Service) InferTarget(ctx context.Context) (string, int, error) { if err != nil { return "", 0, fmt.Errorf("could not read this checkout's remotes: %w", err) } - repo := "" + repos, owners := remoteSlugs(remotes) + if len(repos) == 0 { + return "", 0, fmt.Errorf("no github remote found in this checkout") + } + + // Every remote repository, with every remote owner as a possible head. In a + // fork checkout the branch lives in origin (me/app) while the pull request + // is filed against upstream (owner/app), so taking the first remote and its + // own owner finds nothing and reports that no PR exists. + // + // Asking for this branch specifically rather than listing every PR keeps + // each lookup one request whatever the repository's size, and the usual + // single-remote checkout still costs exactly one. + type match struct { + repo string + pr int + } + var found []match + seen := map[string]bool{} + for _, repo := range repos { + for _, owner := range owners { + query := url.Values{} + query.Set("state", "open") + query.Set("head", owner+":"+branch) + pulls, err := s.gh.ListPulls(ctx, repo, query) + if err != nil { + return "", 0, err + } + for _, pull := range pulls { + key := fmt.Sprintf("%s#%d", strings.ToLower(repo), pull.Number) + if seen[key] { + continue + } + seen[key] = true + found = append(found, match{repo, pull.Number}) + } + } + } + if len(found) == 0 { + return "", 0, fmt.Errorf("no open pull request for %s on branch %s", strings.Join(repos, " or "), branch) + } + if len(found) > 1 { + names := make([]string, 0, len(found)) + for _, m := range found { + names = append(names, fmt.Sprintf("%s#%d", m.repo, m.pr)) + } + return "", 0, fmt.Errorf("branch %s has %d open pull requests (%s); name the one you mean", + branch, len(found), strings.Join(names, ", ")) + } + return found[0].repo, found[0].pr, nil +} + +// remoteSlugs reduces `git remote -v` to the GitHub repositories it names and +// the owners of those repositories, both in first-seen order (so origin leads) +// and deduplicated. +func remoteSlugs(remotes string) (repos, owners []string) { + seenRepo, seenOwner := map[string]bool{}, map[string]bool{} for _, line := range strings.Split(remotes, "\n") { fields := strings.Fields(line) if len(fields) < 2 { continue } - if slug := repoSlugFromRemote(fields[1]); slug != "" { - repo = slug - break + // repoSlugFromRemote is deliberately loose โ€” it exists to MATCH a remote + // against a repo crq already knows, where a wrong guess simply fails to + // match. Here the slug becomes an API lookup, so a local path like + // /home/me/code/app must not turn into a request for code/app. + if !strings.Contains(strings.ToLower(fields[1]), "github.com") { + continue + } + slug := repoSlugFromRemote(fields[1]) + if slug == "" { + continue + } + if key := strings.ToLower(slug); !seenRepo[key] { + seenRepo[key] = true + repos = append(repos, slug) + } + owner := ownerOf(slug) + if key := strings.ToLower(owner); owner != "" && !seenOwner[key] { + seenOwner[key] = true + owners = append(owners, owner) } } - if repo == "" { - return "", 0, fmt.Errorf("no github remote found in this checkout") - } - - // Ask for this branch's PR specifically rather than listing them all: the - // head filter is one request whatever the repository's size. - query := url.Values{} - query.Set("state", "open") - query.Set("head", ownerOf(repo)+":"+branch) - pulls, err := s.gh.ListPulls(ctx, repo, query) - if err != nil { - return "", 0, err - } - if len(pulls) == 0 { - return "", 0, fmt.Errorf("no open pull request for %s on branch %s", repo, branch) - } - if len(pulls) > 1 { - return "", 0, fmt.Errorf("%d open pull requests for %s on branch %s; name the one you mean", len(pulls), repo, branch) - } - return repo, pulls[0].Number, nil + return repos, owners } func gitIn(ctx context.Context, args ...string) (string, error) { diff --git a/internal/crq/target_test.go b/internal/crq/target_test.go index 9e1b531..a52ca08 100644 --- a/internal/crq/target_test.go +++ b/internal/crq/target_test.go @@ -44,3 +44,40 @@ func TestInferTargetAsksForTheBranchesPull(t *testing.T) { t.Fatalf("unfiltered = %+v, want 11,12,13 in order", all) } } + +// A fork checkout has the branch in one repository and the pull request in +// another: origin is me/app, upstream is owner/app, and the PR is filed against +// upstream with head me:branch. Taking the first remote and its own owner asks +// /repos/me/app/pulls?head=me:branch and reports that no PR exists. +func TestRemoteSlugsCoversForkCheckouts(t *testing.T) { + repos, owners := remoteSlugs(`origin git@github.com:me/app.git (fetch) +origin git@github.com:me/app.git (push) +upstream https://github.com/owner/app.git (fetch) +upstream https://github.com/owner/app.git (push)`) + + if len(repos) != 2 || repos[0] != "me/app" || repos[1] != "owner/app" { + t.Fatalf("repos = %v, want both, origin first", repos) + } + if len(owners) != 2 || owners[0] != "me" || owners[1] != "owner" { + t.Fatalf("owners = %v, want both head owners, origin first", owners) + } + + // The ordinary single-remote checkout still yields exactly one of each, so + // inference there still costs exactly one request. + repos, owners = remoteSlugs("origin\tgit@github.com:owner/app.git (fetch)\norigin\tgit@github.com:owner/app.git (push)") + if len(repos) != 1 || len(owners) != 1 { + t.Fatalf("one remote gave repos=%v owners=%v, want one of each", repos, owners) + } + + // A slug here becomes an API lookup, so a remote that is not GitHub at all + // must not produce one โ€” repoSlugFromRemote alone would return "code/app". + if repos, _ := remoteSlugs("origin\t/home/me/code/app (fetch)"); len(repos) != 0 { + t.Errorf("a non-github remote must not become a repository: %v", repos) + } + + // An SSH host alias (git@github.com-work:owner/app.git) is how a second + // account is configured, and is still GitHub. + if repos, _ := remoteSlugs("origin\tgit@github.com-work:owner/app.git (fetch)"); len(repos) != 1 || repos[0] != "owner/app" { + t.Errorf("an ssh host alias must still resolve, got %v", repos) + } +} diff --git a/internal/state/dashboard_test.go b/internal/state/dashboard_test.go index 40b4b01..bd65af6 100644 --- a/internal/state/dashboard_test.go +++ b/internal/state/dashboard_test.go @@ -896,6 +896,36 @@ func TestStatusLine(t *testing.T) { } } + // A stranded reservation with a backlog behind it: the earlier precedence + // check only proved this when the queue happened to be empty, and "stranded + // ... next #8" reads as though the queue is moving. + strandedBacklog := stateWith( + queuedRound("kristofferr/a", 7, 1, now), + queuedRound("kristofferr/a", 8, 2, now), + ) + strandedBacklog.Rounds["kristofferr/a#7"] = func() Round { + r := strandedBacklog.Rounds["kristofferr/a#7"] + r.Phase, r.ReservedAt = PhaseReserved, &reserved + return r + }() + if got := StatusLine(strandedBacklog, StoreConfig{}); strings.Contains(got, "next #") { + t.Errorf("stranded line %q must not point past the stranded round", got) + } + + // A quota-free round stays actionable while the account window is shut โ€” that + // is the whole point of Queue's exemption โ€” so the line must not call it + // blocked and then name what is next in the same breath. + blockedButReady := stateWith(func() Round { + r := queuedRound("kristofferr/a", 9, 1, now) + r.CoOnly = true + return r + }()) + blockedButReady.Account.BlockedUntil = &until + line := StatusLine(blockedButReady, StoreConfig{}) + if strings.Contains(line, "blocked") && strings.Contains(line, "next #") { + t.Errorf("status line %q says blocked and names a next PR at once", line) + } + // It is a status LINE: anything multi-line corrupts the bar it renders into. for _, line := range []string{ StatusLine(New(), StoreConfig{}), diff --git a/internal/state/statusline.go b/internal/state/statusline.go index caf2afb..3952d76 100644 --- a/internal/state/statusline.go +++ b/internal/state/statusline.go @@ -20,12 +20,18 @@ func StatusLine(st State, cfg StoreConfig) string { queue := st.Queue(now, cfg.MinInterval) inFlight := inFlightRounds(st) + // Something is ready to go right now: the queue put it at the front with no + // reason to wait. That outranks the account block, because a quota-free round + // is exactly what Queue leaves actionable while the window is shut โ€” saying + // "blocked" and then "next #7" in the same line contradicts itself. + ready := len(queue) > 0 && queue[0].ReadyAt.IsZero() && queue[0].Why == "" + var parts []string + stranded := firstStranded(st, inFlight) switch { - case firstStranded(st, inFlight) != nil: - s := firstStranded(st, inFlight) - parts = append(parts, fmt.Sprintf("โš  #%d stranded", s.PR)) - case st.Account.BlockedUntil != nil && st.Account.BlockedUntil.After(now): + case stranded != nil: + parts = append(parts, fmt.Sprintf("โš  #%d stranded", stranded.PR)) + case !ready && st.Account.BlockedUntil != nil && st.Account.BlockedUntil.After(now): parts = append(parts, fmt.Sprintf("โณ blocked %dm", minutesUntil(*st.Account.BlockedUntil, now))) case len(inFlight) > 0: parts = append(parts, fmt.Sprintf("๐Ÿ”ฌ #%d reviewing", inFlight[0].PR)) @@ -37,8 +43,10 @@ func StatusLine(st State, cfg StoreConfig) string { if n := len(queue); n > 0 { next := "" - // Name the next PR only when the queue knows which it is; see Queue. - if queue[0].ReadyAt.IsZero() && queue[0].Why == "" { + // Name the next PR only when the queue knows which it is (see Queue) and + // nothing is stranded: a stranded reservation is the whole line, and + // pointing at what comes after it reads as though the queue is moving. + if ready && stranded == nil { next = fmt.Sprintf(" next #%d", queue[0].PR) } parts = append(parts, fmt.Sprintf("%d queued%s", n, next)) diff --git a/llms.txt b/llms.txt index f8633f4..c8452a1 100644 --- a/llms.txt +++ b/llms.txt @@ -45,6 +45,7 @@ finished โ€” is a value crq computes. ```bash crq next OWNER/REPO PR_NUMBER crq next # inside the checkout, the target is inferred + # (same for feedback/loop/cancel: omit ) ``` | `.action` | what to do | diff --git a/skills/coderabbit-queue/SKILL.md b/skills/coderabbit-queue/SKILL.md index 801dad1..ef9dae2 100644 --- a/skills/coderabbit-queue/SKILL.md +++ b/skills/coderabbit-queue/SKILL.md @@ -22,6 +22,7 @@ Do not design one of your own. ```bash crq next "$REPO" "$PR" crq next # inside the checkout: crq finds the PR from the remote and branch + # (so do feedback, loop, cancel โ€” every command that takes a target) ``` | `.action` | what to do |