diff --git a/.gitignore b/.gitignore index 0f92f62..731001f 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,8 @@ dist/ *.log .DS_Store + +# crq report output. Anything left in the working tree reads as unlanded work, +# which would make `crq next` answer "push" forever instead of "done". +crq-next.json +crq-feedback.json diff --git a/README.md b/README.md index d42169b..f9d091a 100644 --- a/README.md +++ b/README.md @@ -45,10 +45,11 @@ yourself anymore β€” you ask `crq`, and `crq`: recent history, and when the next slot opens. - 🌍 **Works across machines** β€” all state lives on GitHub, so agents on your laptop, a server, and a CI box all share the same queue with zero extra infrastructure. -- πŸ” **Drives the round** β€” `crq loop` doesn't just trigger; it waits for CodeRabbit *and* Codex on - the current head, returns every finding as normalized JSON, and reports convergence. +- πŸ” **Drives the round** β€” `crq next` doesn't just trigger; it tracks CodeRabbit *and* Codex on the + current head and answers with the one thing to do now: fix these findings, hold the head, push, + wait until this time, or done. -One agent changes one line β€” `gh pr comment ... @coderabbitai review` becomes `crq loop ` β€” +One agent changes one line β€” `gh pr comment ... @coderabbitai review` becomes `crq next ` β€” and the chaos is gone. > ## ⚠️ Required: turn OFF CodeRabbit auto-review @@ -78,7 +79,7 @@ and the chaos is gone. ```text agent A ─┐ - agent B ─┼─► crq loop + agent B ─┼─► crq next agent C β”€β”˜ β”‚ β–Ό β”Œβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β” asks "any capacity?" @@ -174,12 +175,16 @@ gh pr comment "$PR" --repo "$REPO" --body "@coderabbitai review" # ❌ compete with this: ```bash -crq loop "$REPO" "$PR" > crq-feedback.json # βœ… queues, fires when ready, waits, emits findings +crq next "$REPO" "$PR" # βœ… tells you the one thing to do next ``` -`crq loop` gets in line, fires the review exactly once when CodeRabbit has capacity, waits for -CodeRabbit and Codex on the current head, and writes the findings as JSON. Its exit code tells you -what to do next: `0` converged, `10` actionable findings in the JSON, `2` timed out. +`crq next` gets the PR in line, fires the review exactly once when CodeRabbit has capacity, and +answers with a single instruction: `fix` (findings are attached), `hold` (a required reviewer is +still pending β€” don't move the head), `push`, `wait` until a time crq computed, `done`, or `blocked`. +Call it, do what it says, call it again. It is non-blocking and idempotent, so there is no long-lived +process to babysit. + +`crq loop` is still there as the blocking one-shot form for humans and scripts. --- @@ -269,31 +274,41 @@ Or, if you don't want a long-running process, run `crq autoreview --once` from c ## ⭐ The recommended PR-review loop -This is the autonomous review loop crq was built for. First drain existing work with -`crq feedback`: while `.findings` is non-empty, fix or explicitly decline it, validate locally, and -resolve the addressed thread immediately. Do not wait for or trigger another review while actionable -feedback is open. -`crq loop` enforces that invariant by returning unresolved findings before it queues a fresh -round, and actionable findings take precedence over a feedback timeout. -If any configured feedback bot reports a finding while another required bot is still pending, -`crq loop` returns immediately so you can fix it locally. **Hold the PR head:** do not commit or push, -because changing the head restarts the pending checks. Resolve the addressed thread immediately after -its local fixβ€”thread resolution does not change the head. Leave the queued review alive and poll -`crq feedback` with the same `CRQ_REQUIRED_BOTS`. After every `.reviewed_by` value is true, fix and -resolve the rest, combine all fixes into one commit, and push once. The same policy applies while the -PR is queued for an account-wide review slot. +This is the autonomous review loop crq was built for, and it is one command: + +```bash +crq next "$REPO" "$PR" +``` + +Read `.action`, do exactly that, call it again. + +| `.action` | what to do | +|---|---| +| `fix` | fix `.findings[]`, validate, then `crq resolve` (or `crq decline`) each thread | +| `hold` | **do not commit or push** β€” a required reviewer hasn't answered for this head; call again at `.recheck_after` | +| `push` | the head is released β€” commit and push your fixes once | +| `wait` | nothing to do until `.recheck_after` | +| `done` | converged | +| `blocked` | needs a human; `.reason` says why | + +Every rule this used to spell out is now a value crq computes. Drain-before-review is `fix` coming +before everything else. Hold-the-head is the `hold` action, including the exception where a +CodeRabbit rate-limit degrade releases the head because the queued review will fire on whatever head +exists when the window opens. The wait is `.recheck_after`, derived from the account-quota window, +the round's retry cooldown and the poll interval β€” never a delay you invent. And because `crq next` +returns immediately and is idempotent, an interrupted run loses nothing: call it again. Thread-less review-body summaries from an older commit are informational after a fix is pushed: they cannot be resolved on GitHub, so they do not block a current-head review. That review either re-reports a still-valid finding or supersedes the stale summary. -`crq loop` *is* the review-round primitive β€” it -enqueues, fires when unblocked, waits for both bots on the current head, and emits normalized JSON β€” -so you never hand-poll the GitHub API (which would burn the shared REST quota). Run one per PR, on as -many PRs and machines as you like; they all share the queue without competing. +crq *is* the review-round primitive β€” it enqueues, fires when unblocked, tracks both bots on the +current head, and emits normalized JSON β€” so you never hand-poll the GitHub API (which would burn the +shared REST quota). Run it on as many PRs and machines as you like; they all share the queue without +competing. If the fleet `crq autoreview` daemon is already running, leave it running and do not create another -watcher. A manual `crq loop` and autoreview coordinate through the same idempotent queue entry. After +watcher. A manual `crq next` and autoreview coordinate through the same idempotent queue entry. After a push, whichever path sees the new head first enqueues it; the other re-attaches without spending a second review or posting another CodeRabbit trigger. @@ -301,37 +316,55 @@ second review or posting another CodeRabbit trigger. #!/usr/bin/env bash # review-loop.sh β€” autonomously address CodeRabbit + Codex feedback until a PR converges. # REPO=owner/name PR=123 ./review-loop.sh +# +# The whole loop is: ask crq what to do, do it, ask again. No exit codes to +# interpret, no sleep to guess, no rule about when the head may move. set -uo pipefail REPO="${REPO:?set REPO=owner/name}"; PR="${PR:?set PR=}" +# Keep the report OUT of the checkout: crq decides push-vs-done by asking whether +# the working tree holds changes the PR head lacks, so a report written next to +# your code is itself uncommitted work and the loop can never reach "done". +OUT="$(mktemp -t crq-next.XXXXXX.json)"; trap 'rm -f "$OUT"' EXIT + while :; do - # crq loop: enqueue + rate-coordinated trigger + wait for feedback. Exit codes: - # 0 = converged 10 = actionable findings in JSON 2 = timed out - crq loop "$REPO" "$PR" > crq-feedback.json; rc=$? - case "$rc" in - 0) echo "βœ… $REPO#$PR converged."; break ;; - 2) echo "timed out; not pushing a stale-feedback round"; sleep 60; continue ;; - 10) : ;; # fall through and fix - *) echo "crq loop error ($rc)"; exit "$rc" ;; + crq next "$REPO" "$PR" > "$OUT" || exit 1 + action=$(jq -r .action "$OUT") + + case "$action" in + done) echo "βœ… $REPO#$PR converged."; break ;; + blocked) echo "β›” $(jq -r .reason "$OUT")"; exit 1 ;; + + fix) + # Read findings, fix the real ones, run your tests/linters. + jq -r '.findings[] | "\(.severity) \(.path // "-"):\(.line // 0) β€” \(.title)"' "$OUT" + # ... apply fixes and validate ... + + # Resolve the threads you addressed; record why for any you decline. + threads=$(jq -r '.findings[] | select(.thread_id != null) | .thread_id' "$OUT") + # shellcheck disable=SC2086 -- thread ids are opaque tokens with no spaces + [ -n "$threads" ] && crq resolve $threads + # crq decline --reason "why this one is declined" + ;; + + push) git add -A && git commit -m "address review feedback" && git push ;; + + hold|wait) + # Don't compute a delay β€” hand the wait over. crq wait blocks until there + # is something to do and exits, so there is no timestamp to parse and no + # sleep to guess. (Parsing .recheck_after yourself needs GNU date; this + # doesn't.) + echo "$action: $(jq -r .reason "$OUT")" + crq wait "$REPO" "$PR" > "$OUT" || exit 1 + ;; esac - - # Read findings, fix the real ones, run your tests/linters, then resolve them immediately. - jq -r '.findings[] | "\(.severity) \(.path // "-"):\(.line // 0) β€” \(.title)"' crq-feedback.json - # ... apply fixes and validate ... - - # Resolve the threads you addressed; record why for any you decline. - jq -r '.findings[] | select(.thread_id != null) | .thread_id' crq-feedback.json \ - | xargs -I{} crq resolve "$REPO" "$PR" --thread {} - # crq decline "$REPO" "$PR" --thread --reason "why this one is declined" - # If reviewers are still pending, keep the head frozen and resume with crq feedback. - # After all required reviewers finish: fix/resolve the rest, then git commit && git push once. done ``` [`examples/review-loop.sh`](examples/review-loop.sh) is a minimal **one-shot wrapper** around the -same contract β€” it runs `crq loop` once, prints what to do for each exit code, and exits with crq's -code. It is the building block, not the full autonomous loop above: drop it inside your own -`while`/fix/push/resolve cycle (or let your agent drive the loop). +same contract β€” it runs `crq next` once and prints what to do for the action it returns. It is the +building block, not the full autonomous loop above: drop it inside your own `while` cycle (or let +your agent drive the loop). > πŸ’‘ **Watching the line:** run `crq status` any time to see the queue, what's in flight, and the > next slot. Or open the gate **issue** on GitHub β€” it **is** the live dashboard. @@ -347,10 +380,11 @@ least 10 minutes of silence. ## Commands ```bash -crq loop # ⭐ enqueue + fire + wait for both bots + emit JSON findings (use in loops) +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 resolve --thread [...] # resolve addressed review threads -crq decline --thread --reason "" [--resolve] # record why a finding is declined +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 # (--no-incremental = first review only; --once = single pass for cron) crq status # show the dashboard: queue, in-flight, quota, next slot @@ -358,12 +392,13 @@ crq doctor # JSON readiness report (gh/auth/config/CLI) β€” never crq preflight [...] # run the local CodeRabbit CLI pre-push and normalize its JSON crq cancel # take a PR out of the line crq init # first-time setup of the gate repo -crq debug # diagnosis only β€” review loops should use crq loop +crq debug # diagnosis only β€” review loops should use crq next crq version # print the version crq help [command] # help, optionally for one command ``` -`` is `owner/name`; `` is the number. **`crq loop` exit codes:** `0` converged or no +`` is `owner/name`; `` is the number. **`crq next` always exits 0** β€” read `.action`, not +the exit code. **`crq loop` exit codes:** `0` converged or no actionable findings, `10` actionable findings returned in `.findings[]`, `2` timed out waiting for feedback. crq keys resolution off GitHub's own thread state, so a finding keeps reappearing in `feedback`/`loop` until its thread is resolved (or declined-and-resolved) on GitHub. @@ -547,24 +582,24 @@ serialize reviews that don't actually compete. If you're an autonomous agent running a PR-review loop, here's everything you need: -- **The one rule:** never post `@coderabbitai review` yourself. To run a review round, use - `crq loop "" ""` β€” it blocks until feedback lands and writes findings to stdout. -- **Never hand-poll GitHub for review status.** Don't loop `gh api .../pulls/N/reviews|comments` - waiting for a review β€” that drains the shared account-wide REST quota (also spent by the - `autoreview` daemon and every other agent) and competes with crq's own polling. Use `crq loop` - (waits + returns findings), `crq feedback` (current findings, no trigger), or `crq status`. -- **Exit codes:** `0` converged β†’ done; `10` β†’ read `.findings[]`, fix valid ones, validate locally, - and resolve addressed threads immediately. If any `.reviewed_by` value is false, **hold the - head**β€”do not commit or push. After all required bots are true, fix/resolve the rest, commit/push - once, then loop again; - `2` β†’ timed out, don't push stale-feedback fixes. -- **Resolve / decline:** after fixing a finding, `crq resolve --thread `. If you're - declining one, record why with `crq decline --thread --reason "…"` instead of - leaving it silently open. -- **A long wait is not a hang.** During queue/rate-limit waits, feedback waits, and network - outages, crq logs progress to **stderr** (queue reason, per-bot `reviewed` status, `github - unreachable … offline …` / `reachable again`). It keeps retrying through an internet drop until - connectivity returns (no timeout by default), so don't kill it β€” watch stderr. +- **The loop is one command.** Call `crq next "" ""`, do exactly what `.action` + says, call it again. Don't design a loop of your own, and don't read the exit code β€” `next` + always exits 0 on success and the answer is `.action`: + `fix` Β· `hold` Β· `push` Β· `wait` Β· `done` Β· `blocked`. +- **Never post `@coderabbitai review` yourself.** crq is the only trigger, because the review limit + is account-wide and direct posts stampede it. +- **On `wait` or `hold`, hand the wait over.** Run `crq wait "" ""` as your + harness's background task and end your turn: it blocks until there is something to do and its + **exit is your wake event**. It holds no round, so killing it costs only the process. Never + invent a delay, never poll in-chat, and never loop `gh api .../pulls/N/reviews|comments` β€” that + drains the shared REST quota the daemon and every other agent are also spending. +- **Never choose when to push.** `hold` vs `push` is crq's answer, and it already accounts for the + rate-limit degrade and the quiet period after a review lands. +- **Resolve / decline:** after fixing a finding, `crq resolve ...` (pass them all at + once). If you're declining one, `crq decline --reason "…"` β€” that resolves it too, + because a thread left open keeps its finding actionable; `--keep-open` overrides. +- **Don't narrate the wait.** Report real state changes β€” findings, a push, convergence, a block β€” + not elapsed time. - **Setup check:** run `crq doctor`; if config is missing, do the Quick Start (install + `crq init`). The installer puts the **[Codex skill](skills/coderabbit-queue/SKILL.md)** on the local skill path diff --git a/cmd/crq/main.go b/cmd/crq/main.go index 15a6662..d6816d4 100644 --- a/cmd/crq/main.go +++ b/cmd/crq/main.go @@ -117,6 +117,53 @@ func run(ctx context.Context, args []string) int { } printJSON(report) return 0 + case "next": + if bad, found := unknownFlag(args[1:], "--wait"); found { + 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]")) + return 1 + } + if err := cfg.RequireState(); 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 { + report, err = service.Next(ctx, repo, pr) + } + if err != nil { + fatal(err) + return 1 + } + printJSON(report) + // The action field is the whole contract: exit 0 for every action so a + // 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 ")) + return 1 + } + if err := cfg.RequireState(); err != nil { + fatal(err) + return 1 + } + report, err := service.WaitForAction(ctx, repo, pr) + if err != nil { + fatal(err) + return 1 + } + printJSON(report) + // 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 { @@ -137,11 +184,11 @@ func run(ctx context.Context, args []string) int { case "resolve": threads, ok := parseResolveArgs(args[1:]) if !ok { - fatal(errors.New("usage: crq resolve --thread [--thread ...]")) + fatal(errors.New("usage: crq resolve [...]")) return 1 } if len(threads) == 0 { - fatal(errors.New("usage: crq resolve --thread [--thread ...]")) + fatal(errors.New("usage: crq resolve [...]")) return 1 } result, err := service.ResolveThreads(ctx, threads) @@ -154,7 +201,7 @@ func run(ctx context.Context, args []string) int { case "decline": threads, reason, resolve, ok := parseDeclineArgs(args[1:]) if !ok || len(threads) == 0 || strings.TrimSpace(reason) == "" { - fatal(errors.New(`usage: crq decline --thread [--thread ...] --reason "" [--resolve]`)) + fatal(errors.New(`usage: crq decline [...] --reason "" [--keep-open]`)) return 1 } result, err := service.DeclineThreads(ctx, threads, reason, resolve) @@ -262,26 +309,38 @@ 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 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 -ONE PR ROUND - 1. Run: crq loop > crq-feedback.json - 2. If exit 10, read .findings[], fix only valid findings, and validate locally. - 3. Resolve each addressed thread immediately after its local fix; do not wait for a push. - 4. If any .reviewed_by value is false, HOLD THE HEAD: do not commit or push. - 5. After every required bot is true, fix/resolve the rest, then commit and push once. - 6. Repeat crq loop until exit 0. Never post @coderabbitai review directly. +DRIVING A PR REVIEW + Call crq next, do exactly what .action says, call it again. That is the whole loop. + + fix fix .findings[], validate, then crq resolve (or crq decline) each thread + hold do NOT push: a required reviewer is pending; call again at .recheck_after + push the head is released β€” commit and push your fixes once + wait nothing to do; call again at .recheck_after + done converged + blocked needs a human; .reason says why + + crq next is non-blocking and idempotent, so nothing is lost if it is interrupted. + On wait or hold, hand the delay to crq wait β€” run it as your harness's background + task and let its EXIT wake you. It owns nothing, so being killed costs only itself. + Never invent a delay of your own, and never post @coderabbitai review directly. USAGE crq init initialize state in CRQ_REPO + crq next [--wait] emit the single next action as JSON (--wait blocks) + 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 resolve --thread [...] + crq resolve [...] resolve addressed GitHub review threads - crq decline --thread [...] --reason "" [--resolve] + crq decline [...] --reason "" [--keep-open] reply on a thread to record why a finding is declined + (resolves it; --keep-open leaves it open) crq autoreview [--once] [--no-incremental] keep open PRs reviewed, rate-coordinated crq preflight [--type all|committed|uncommitted] [--base ] @@ -293,6 +352,7 @@ USAGE maintenance tools; not for normal review loops EXIT CODES + next: always 0 on success β€” read .action, not the exit code loop: 0 converged/no actionable findings/skipped, 10 actionable feedback, 2 timeout Configure with environment variables or ~/.config/crq/env. CRQ_REPO points at the gate repo. @@ -303,6 +363,67 @@ Use "crq help " for command-specific guidance. func commandHelp(command string) { switch command { + case "next": + fmt.Print(`crq next [--wait] + +The agent loop, in one command: crq answers what to do next about this PR, you do +exactly that, then you call it again. Non-blocking, idempotent, and it advances the +queue by one step as a side effect β€” so a PR outside the autoreview fleet still +progresses, and an interrupted caller loses nothing by calling again. + +Always exits 0 on success. Read .action; the exit code carries no information. + + fix .findings[] are actionable for the current head. Fix them, validate, then + crq resolve each addressed .thread_id (or crq decline with a reason). + hold You have work to land but a required reviewer has not answered for this + head. Do NOT commit or push β€” that restarts the review. Resolving threads + is still fine. Call again at .recheck_after. + push The head is released. Commit and push the accumulated fixes once. + wait Nothing to do until .recheck_after. + done Converged: no findings, every required reviewer answered. + blocked Needs a human (.reason says why, e.g. the PR was closed). + +Fields: + action the instruction β€” the entire contract + reason why, in one line + recheck_after when to call again (hold and wait). crq computes this from the + account-quota window, the round's retry cooldown and the poll + interval. Never substitute a delay of your own. + pending[] required reviewers with no evidence for this head + findings[] actionable feedback (fix); same shape as crq feedback + local_work whether crq saw changes the PR head lacks β€” what separates push + from done. Run crq next inside the repository checkout so this + is accurate; local_work_reason says when it could not be. + +--wait blocks through the states you cannot act on and returns the first actionable +instruction. It shares one code path with the non-blocking form. + +Never post @coderabbitai review directly; crq is the only trigger. +`) + case "wait": + fmt.Print(`crq wait + +Block until there is something to DO about this PR, then print that instruction +(the same JSON as crq next) and exit 0. + +This is how an ephemeral agent waits. Run it as your harness's background task and +end your turn: its EXIT is the wake event, so you burn no tokens idling and invent +no delay. It holds no round, so if it is killed the round is untouched β€” just run +it again, or call crq next. + +It is read-only in the steady state, but NOT unconditionally: if nothing is +advancing this PR (no round for the head, or no daemon holding the leader lease) +it drives the queue itself rather than wait for nobody, which can request a +review and spend account quota. + +It returns on fix, push, done and blocked. wait and hold are the two states it +waits THROUGH, because they mean "come back later" and that is its whole job. + +While idle it watches the shared state ref with a conditional request, which costs +no rate-limit quota, and re-evaluates when the queue moves. If no autoreview daemon +holds the leader lease it drives the queue itself instead, which works but spends +more of the shared budget β€” run the daemon. +`) case "loop": fmt.Print(`crq loop @@ -352,7 +473,7 @@ Each finding has: Sources include review_thread, review_comment, review_body, review_prompt, and issue_comment. `) case "resolve": - fmt.Print(`crq resolve --thread [--thread ...] + fmt.Print(`crq resolve [...] Resolve only GitHub review threads that were actually addressed by the latest fix. Leave declined, stale, incorrect, or deferred findings unresolved. @@ -360,14 +481,19 @@ Leave declined, stale, incorrect, or deferred findings unresolved. Thread IDs come from .findings[].thread_id in crq loop/feedback output. `) case "decline": - fmt.Print(`crq decline --thread [--thread ...] --reason "" [--resolve] + fmt.Print(`crq decline [...] --reason "" [--keep-open] Record on the PR why a finding is being declined: posts the reason as a reply on each review thread. Use this instead of silently leaving a finding unaddressed, so the next reviewer (and CodeRabbit) can see the decision. -By default the thread stays unresolved (an on-the-record disagreement). Pass ---resolve to also close it ("won't fix"). Thread IDs come from .findings[].thread_id. +Declining RESOLVES the thread, because crq reads GitHub's resolution state: a +thread left open keeps its finding actionable, so crq next would repeat "fix" +forever and never reach push or done. The disagreement is not lost β€” if the bot +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 "autoreview", "auto": fmt.Print(`crq autoreview [--once] [--no-incremental] @@ -400,7 +526,11 @@ Exit codes: 0 clean/no local findings 10 local findings returned in .findings[] 1 setup, auth, CLI, or parsing error - 2 timeout + 2 come back later: a timeout, or the CodeRabbit account is rate-limited + (.status is "rate_limited", with .retry_after and .error_type) + +The local CLI spends the same account quota as PR reviews, so a local block is +evidence about that shared quota β€” read .retry_after rather than re-running. Use crq loop for queued GitHub PR reviews. `) @@ -489,59 +619,122 @@ func isHelpArg(arg string) bool { return arg == "-h" || arg == "--help" || arg == "help" } -func parseResolveArgs(args []string) ([]string, bool) { - var positional []string - var threads []string - for i := 0; i < len(args); i++ { - switch args[i] { - case "--thread": - if i+1 >= len(args) { - return nil, false - } - threads = append(threads, args[i+1]) - i++ - default: - positional = append(positional, args[i]) +// hasFlag reports whether args contains the exact flag. +func hasFlag(args []string, flag string) bool { + for _, arg := range args { + if arg == flag { + return true } } - if len(positional) != 0 && len(positional) < 2 { - return nil, false + return false +} + +// unknownFlag returns the first flag in args that is not in allowed. +// +// positional() drops anything starting with "-", so without this a mistyped +// --wiat silently ran the non-blocking form: the caller gets a plausible report +// and waits forever for a blocking call that never happened. A typo must fail. +func unknownFlag(args []string, allowed ...string) (string, bool) { + for _, arg := range args { + if !strings.HasPrefix(arg, "-") { + continue + } + known := false + for _, candidate := range allowed { + if arg == candidate { + known = true + break + } + } + if !known { + return arg, true + } } - if len(positional) > 2 { - threads = append(threads, positional[2:]...) + return "", false +} + +// positional drops flag arguments so repoPR sees only . +func positional(args []string) []string { + out := make([]string, 0, len(args)) + for _, arg := range args { + if !strings.HasPrefix(arg, "-") { + out = append(out, arg) + } } - return threads, true + return out +} + +func parseResolveArgs(args []string) ([]string, bool) { + threads, _, _, ok := parseThreadCommand(args, false) + return threads, ok } func parseDeclineArgs(args []string) (threads []string, reason string, resolve, ok bool) { + return parseThreadCommand(args, true) +} + +// parseThreadCommand parses the shape shared by `crq resolve` and `crq decline`: +// any number of thread IDs, written bare or behind --thread. +// +// Thread node IDs are globally unique, so the this command used to +// demand never identified anything β€” ResolveThreads discarded them. They are +// still accepted so existing call sites keep working, and dropped here. Taking +// IDs bare is what lets a caller clear a whole round in one process instead of +// one subprocess per thread. +// +// An unrecognized flag is an error rather than a positional: a typo like +// `--resove` must fail loudly, not silently become a thread ID. +func parseThreadCommand(args []string, allowReason bool) (threads []string, reason string, resolve, ok bool) { + // Declining resolves the thread unless the caller asks to keep it open. + // Leaving it open by default made `crq next` repeat `fix` forever: crq keys + // off GitHub's resolution state, so a documented decline cleared nothing and + // the loop could never reach push or done. A bot that disagrees still gets + // heard β€” a contested reply is re-surfaced as its own finding. + resolve = allowReason + var keepOpen bool var positional []string for i := 0; i < len(args); i++ { - switch args[i] { - case "--thread": + arg := args[i] + switch { + case arg == "--thread": if i+1 >= len(args) { return nil, "", false, false } threads = append(threads, args[i+1]) i++ - case "--reason": + case allowReason && arg == "--reason": if i+1 >= len(args) { return nil, "", false, false } reason = args[i+1] i++ - case "--resolve": + case allowReason && arg == "--resolve": + // Kept as an accepted no-op: resolving is now the default, and + // failing on a flag that used to be required would break callers. resolve = true + case allowReason && arg == "--keep-open": + keepOpen = true + case strings.HasPrefix(arg, "-"): + return nil, "", false, false default: - positional = append(positional, args[i]) + positional = append(positional, arg) } } - if len(positional) != 0 && len(positional) < 2 { - return nil, "", false, false + if keepOpen { + resolve = false } - if len(positional) > 2 { - threads = append(threads, positional[2:]...) + return append(threads, dropLegacyTarget(positional)...), reason, resolve, true +} + +// dropLegacyTarget removes a leading "owner/repo" plus its PR number β€” the +// arguments these commands used to require β€” leaving only thread IDs. +func dropLegacyTarget(positional []string) []string { + if len(positional) >= 2 && strings.Contains(positional[0], "/") { + if _, err := strconv.Atoi(positional[1]); err == nil { + return positional[2:] + } } - return threads, reason, resolve, true + return positional } func printJSON(value any) { @@ -637,7 +830,7 @@ func doctor(ctx context.Context) doctorReport { "crq preflight --type uncommitted", "crq loop ", "crq feedback ", - "crq resolve --thread ", + "crq resolve ", "crq autoreview --once", }, Recommendations: []string{}, diff --git a/cmd/crq/main_test.go b/cmd/crq/main_test.go new file mode 100644 index 0000000..a7930cb --- /dev/null +++ b/cmd/crq/main_test.go @@ -0,0 +1,114 @@ +package main + +import ( + "strings" + "testing" +) + +// The thread commands take IDs bare so a caller can clear a whole round in one +// process. The transcripts that motivated this were full of shell loops running +// one `crq resolve` subprocess per thread, every round. +func TestParseThreadCommand(t *testing.T) { + const ( + t1 = "PRRT_kwDOAAAAAA1" + t2 = "PRRT_kwDOAAAAAA2" + ) + cases := []struct { + name string + args []string + reason bool + want []string + wantRsn string + wantRes bool + wantErr bool + }{ + {name: "bare ids", args: []string{t1, t2}, want: []string{t1, t2}}, + {name: "flag form still works", args: []string{"--thread", t1, "--thread", t2}, want: []string{t1, t2}}, + {name: "mixed forms", args: []string{"--thread", t1, t2}, want: []string{t1, t2}}, + { + // The old signature demanded a target these commands never used: + // thread node IDs are globally unique. + name: "legacy repo and pr are dropped", + args: []string{"owner/repo", "123", t1}, + want: []string{t1}, + }, + { + name: "legacy target with flag threads", + args: []string{"owner/repo", "123", "--thread", t1}, + want: []string{t1}, + }, + { + // A repo-shaped first arg is only a target when a PR number follows, + // so an ID that happens to contain "/" is not eaten. + name: "slashed id without a pr number is a thread", + args: []string{"weird/id", t1}, + want: []string{"weird/id", t1}, + }, + {name: "dangling --thread is an error", args: []string{"--thread"}, wantErr: true}, + { + name: "unknown flag is an error, not a thread id", + args: []string{"--treahd", t1}, wantErr: true, + }, + { + // Declining resolves by default: a thread left open keeps its finding + // actionable, so the loop would repeat `fix` forever. + name: "decline resolves by default", + args: []string{t1, "--reason", "not a real issue"}, reason: true, + want: []string{t1}, wantRsn: "not a real issue", wantRes: true, + }, + { + name: "decline --keep-open leaves the disagreement open", + args: []string{t1, "--reason", "still discussing", "--keep-open"}, reason: true, + want: []string{t1}, wantRsn: "still discussing", wantRes: false, + }, + { + // --resolve used to be required; accepting it as a no-op keeps existing + // callers working now that it is the default. + name: "legacy --resolve still accepted", + args: []string{t1, "--reason", "no", "--resolve"}, reason: true, + want: []string{t1}, wantRsn: "no", wantRes: true, + }, + { + // resolve has no --reason, so passing one must fail rather than be + // swallowed as a positional. + name: "reason flag is rejected by resolve", + args: []string{t1, "--reason", "x"}, wantErr: true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + threads, reason, resolve, ok := parseThreadCommand(tc.args, tc.reason) + if tc.wantErr { + if ok { + t.Fatalf("parseThreadCommand(%v) = %v, want an error", tc.args, threads) + } + return + } + if !ok { + t.Fatalf("parseThreadCommand(%v) failed, want %v", tc.args, tc.want) + } + if strings.Join(threads, ",") != strings.Join(tc.want, ",") { + t.Errorf("threads = %v, want %v", threads, tc.want) + } + if reason != tc.wantRsn { + t.Errorf("reason = %q, want %q", reason, tc.wantRsn) + } + if resolve != tc.wantRes { + t.Errorf("resolve = %v, want %v", resolve, tc.wantRes) + } + }) + } +} + +// A mistyped flag used to be dropped as a non-positional, so `--wiat` ran the +// non-blocking form and looked like success. +func TestUnknownFlag(t *testing.T) { + if _, found := unknownFlag([]string{"owner/repo", "1", "--wait"}, "--wait"); found { + t.Error("a known flag must be accepted") + } + bad, found := unknownFlag([]string{"owner/repo", "1", "--wiat"}, "--wait") + if !found || bad != "--wiat" { + t.Errorf("unknownFlag = (%q, %v), want (--wiat, true)", bad, found) + } +} diff --git a/examples/review-loop.sh b/examples/review-loop.sh index 7abd866..62cf4f3 100755 --- a/examples/review-loop.sh +++ b/examples/review-loop.sh @@ -1,36 +1,54 @@ #!/usr/bin/env bash -# Minimal agent wrapper around crq's JSON/exit-code contract. +# Minimal agent wrapper around crq's next-action contract. +# +# One step of the loop: ask crq what to do about this PR and print it. There is +# no exit code to interpret and no delay to invent β€” `crq next` answers both. +# Drop this inside your own while-loop, or let your agent drive it. set -euo pipefail REPO="${REPO:?set REPO=owner/name}" PR="${PR:?set PR=}" -OUT="${OUT:-crq-feedback.json}" +# Default OUT lives OUTSIDE the checkout on purpose. crq decides push-vs-done by +# asking whether the working tree holds changes the PR head lacks, so a report +# written into the repository is itself uncommitted work: the loop would then +# read as "push" forever and could never reach "done". +if [ -n "${OUT:-}" ]; then + # A caller-provided path is theirs to keep; only clean up what we created. + : +else + OUT="$(mktemp -t crq-next.XXXXXX.json)" + trap 'rm -f "$OUT"' EXIT +fi -set +e -crq loop "$REPO" "$PR" > "$OUT" -rc=$? -set -e +crq next "$REPO" "$PR" > "$OUT" +action=$(jq -r .action "$OUT") +reason=$(jq -r '.reason // ""' "$OUT") +recheck=$(jq -r '.recheck_after // ""' "$OUT") -case "$rc" in - 0) - echo "converged or no actionable findings; see $OUT" - ;; - 10) - echo "actionable findings written to $OUT" - echo "fix valid findings and validate locally" - echo "resolve each addressed thread immediately after its local fix:" +echo "action: $action${reason:+ β€” $reason}" + +case "$action" in + fix) + echo "fix the findings in $OUT and validate locally, then resolve each addressed thread:" echo " jq -r '.findings[] | select(.thread_id != null) | .thread_id' '$OUT'" - echo " crq resolve '$REPO' '$PR' --thread THREAD_ID" - echo "if any .reviewed_by value is false: HOLD THE HEAD; do not commit or push" - echo "after every required bot is true: fix/resolve the rest, then commit/push once" + echo " crq resolve THREAD_ID [THREAD_ID...]" + echo " crq decline THREAD_ID --reason 'why not addressed'" + ;; + hold) + echo "DO NOT commit or push β€” moving the head restarts the pending review" + echo "pending reviewers: $(jq -r '(.pending // []) | join(", ")' "$OUT")" + echo "call crq next again at $recheck" ;; - 2) - echo "timed out waiting for feedback; do not push a stale-feedback round; see $OUT" + push) + echo "the head is released; commit and push your accumulated fixes once, then call crq next again" ;; - *) - echo "crq loop failed with exit $rc" + wait) + echo "nothing to do; call crq next again at $recheck" + ;; + done) + echo "converged" + ;; + blocked) + echo "needs a human" ;; esac - -# Propagate crq's exit code so automation can branch on findings/timeout/convergence. -exit "$rc" diff --git a/internal/crq/feedback.go b/internal/crq/feedback.go index 5e38910..2e93415 100644 --- a/internal/crq/feedback.go +++ b/internal/crq/feedback.go @@ -26,6 +26,21 @@ type FeedbackReport struct { ReviewedBy map[string]bool `json:"reviewed_by"` Findings []dialect.Finding `json:"findings"` CheckedAt time.Time `json:"checked_at"` + // Open is whether the PR was open in the same observation Head and + // ReviewedBy came from. It is deliberately not serialized β€” the feedback + // JSON contract is frozen β€” and exists so a caller deciding an action reads + // head, open and evidence from ONE snapshot rather than re-reading the pull. + Open bool `json:"-"` + // HeadRef and HeadRepo describe where the PR's branch actually lives. On a + // fork PR that repository is not the one the PR is filed against, and a + // contributor's checkout only has a remote for it. Not serialized: the + // feedback JSON contract is frozen. + HeadRef string `json:"-"` + HeadRepo string `json:"-"` + // LastEvidenceAt is when the newest review from a feedback bot landed. It + // anchors the settle window for a caller that holds no state between calls: + // "quiet since" is derivable, where the loop's in-process settledAt is not. + LastEvidenceAt time.Time `json:"-"` // CodeRabbitDeferred marks a round degraded to Codex-only while the // CodeRabbit account is rate-limited: Codex feedback is authoritative for // this round, the CodeRabbit review stays queued and fires after @@ -87,6 +102,9 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe Repo: repo, PR: pr, Head: head, + Open: obs.eng.Open, + HeadRef: pull.Head.Ref, + HeadRepo: NormalizeRepo(pull.Head.Repo.FullName), ReviewedBy: map[string]bool{}, Findings: []dialect.Finding{}, CheckedAt: now, @@ -107,6 +125,29 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe } completion := engine.Completion(completionRound, obs.eng, s.policy()) report.ReviewedBy = completion.ReviewedBy + // Completion does not always arrive as a review: a clean-summary comment, a + // paired completion reply and a co-reviewer check run all satisfy it. Anchor + // the settle window on the newest of ANY of them, or a round completed by a + // comment would settle against a stale timestamp and converge instantly. + evidenceBots := dialect.BotSet(unionBots(s.cfg.FeedbackBots, s.cfg.RequiredBots)) + noteEvidence := func(at time.Time) { + if at.After(report.LastEvidenceAt) { + report.LastEvidenceAt = at + } + } + for _, review := range obs.reviews { + if dialect.InBots(evidenceBots, review.User.Login) { + noteEvidence(review.SubmittedAt) + } + } + for _, comment := range obs.comments { + if dialect.InBots(evidenceBots, comment.User.Login) { + noteEvidence(comment.UpdatedAt) + } + } + for _, check := range obs.eng.Checks { + noteEvidence(check.CompletedAt) + } verdictCutoff := anchorCutoff if verdictCutoff.IsZero() { // Not fired yet (queued behind another PR): without a fire anchor the diff --git a/internal/crq/next.go b/internal/crq/next.go new file mode 100644 index 0000000..d72155a --- /dev/null +++ b/internal/crq/next.go @@ -0,0 +1,453 @@ +package crq + +import ( + "context" + "os/exec" + "strings" + "time" + + "github.com/kristofferR/coderabbit-queue/internal/dialect" + "github.com/kristofferR/coderabbit-queue/internal/engine" + ghapi "github.com/kristofferR/coderabbit-queue/internal/gh" +) + +// NextReport is what `crq next` prints: one instruction, and the data needed to +// carry it out. The `action` field is the entire contract β€” a caller reads it, +// does exactly that, and calls again. Nothing else needs interpreting, which is +// why this command exits 0 for every action and reserves non-zero for hard +// failures alone. +type NextReport struct { + Action string `json:"action"` + Reason string `json:"reason,omitempty"` + Repo string `json:"repo"` + PR int `json:"pr"` + Head string `json:"head,omitempty"` + + // RecheckAfter is when to call `crq next` again β€” the ONE time field, set + // for both hold and wait so there is never a question of which to read. crq + // computes it; a caller must never invent a delay of its own. + RecheckAfter *time.Time `json:"recheck_after,omitempty"` + + // Pending lists the required reviewers with no evidence for this head. + Pending []string `json:"pending,omitempty"` + Findings []dialect.Finding `json:"findings"` + + ReviewedBy map[string]bool `json:"reviewed_by,omitempty"` + // LocalWork records whether crq saw changes the PR head does not have. It + // is what separates "push" from "done"; when crq is not run inside the + // repository it is false and LocalWorkReason says so. + LocalWork bool `json:"local_work"` + LocalWorkReason string `json:"local_work_reason,omitempty"` + CheckedAt time.Time `json:"checked_at"` +} + +// Next answers "what should I do about this PR right now?" in one +// non-blocking call. +// +// It replaces the agent-side protocol that `crq loop` documented but could not +// enforce: drain findings before starting a round, hold the head while a +// required reviewer is pending, pick a sensible delay. Each of those is now a +// value in the returned report rather than a judgement call at the call site. +// +// The call also advances the queue by one pump step, so a PR in a repository +// outside the autoreview fleet still progresses without a daemon β€” and because +// every write is CAS'd, running it alongside the daemon is safe. Being +// non-blocking is the point: there is no long-lived process for a harness to +// kill, and a caller that dies mid-loop simply calls again. +func (s *Service) Next(ctx context.Context, repo string, pr int) (NextReport, error) { + repo = NormalizeRepo(repo) + report := NextReport{Repo: repo, PR: pr, Findings: []dialect.Finding{}, CheckedAt: s.clock()} + + // Observe BEFORE publishing anything. Enqueue makes this head fire-eligible + // in shared state, and an autoreview daemon pumping concurrently can claim + // and fire it in the gap β€” CAS protects each individual write, not this + // sequence. Deciding first means the drain-first rule is enforced before the + // round is exposed at all, rather than only in this process's own pump. + report, action, feedback, err := s.nextFromState(ctx, repo, pr) + if err != nil { + return report, err + } + + // Undrained feedback for THIS head: publish nothing. Another review of the + // same head would spend account quota to be told what the caller is already + // holding. + // + // The gate is deliberately narrow. Findings CARRIED from an older commit β€” + // an unresolved thread the caller may well have already fixed β€” must not + // stop a new head from being reviewed, or an unresolvable one deadlocks the + // PR forever and no review is ever requested for the code that replaced it. + if len(engine.FindingsOnHead(action.Findings, report.Head)) > 0 { + return report, nil + } + + // The caller is mid-flight: it is about to move the head, or it is holding + // fixes for a finding it was just handed. Queueing a review now spends a + // window on code that is being replaced, and the round is superseded moments + // later anyway. A carried thread reaches here with FindingsOnHead empty, so + // this guard β€” not that one β€” is what stops a review firing ahead of the + // fixes for it. + if action.Kind == engine.ActionPush || (action.Kind == engine.ActionFix && report.LocalWork) { + return report, nil + } + + // A dry run reports decisions and writes nothing. Enqueue is a CAS write + // plus a dashboard sync, so it has to be skipped here rather than left to + // the dry-run-aware apply path further down. + if s.cfg.DryRun { + return report, nil + } + + // Nothing left to drain, so record the round for this head (idempotent; + // supersedes on a new head) and advance the queue one step. + enqueued, err := s.Enqueue(ctx, repo, pr) + if err != nil { + return report, err + } + // Enqueue re-reads the head. If it moved in between, every conclusion above + // describes a head that is no longer current β€” and returning `done` for it + // would stop a caller just as an unreviewed head was queued. Say so and let + // the next call decide on one snapshot. + if enqueued.Head != "" && report.Head != "" && enqueued.Head != report.Head { + report.Head = enqueued.Head + report.Action = string(engine.ActionWait) + report.Reason = "the head moved while deciding; re-reading it" + report.Findings = []dialect.Finding{} + at := s.clock().Add(s.waitTick()).UTC() + report.RecheckAfter = &at + return report, nil + } + startedCoReview, err := s.advance(ctx, repo, pr, feedback) + if err != nil { + // Throttling is expected and self-clearing: the instruction above still + // stands on the observation already taken, and the next call advances. + // Anything else is a real failure β€” a token that cannot post the review + // command, say β€” and returning a cheerful `wait` would have callers + // retrying forever against something no amount of waiting fixes. + if _, throttled := ghapi.ThrottleWait(err); !throttled { + return report, err + } + if s.log != nil { + s.log.Printf("throttled while advancing %s: %v", QueueKey(repo, pr), err) + } + } + // A co-review just started answers in minutes, but the schedule above was + // computed when nothing was in flight β€” for an account-blocked round that + // means the quota window, hours away. Bring the caller back on the short + // cadence instead of sleeping through the findings this call set in motion. + if startedCoReview { + at := s.clock().Add(s.waitTick()).UTC() + if report.RecheckAfter == nil || report.RecheckAfter.After(at) { + report.RecheckAfter = &at + } + } + return report, nil +} + +// settleUntil is when a convergence verdict may be trusted: the newest review +// plus the configured quiet period. Nil when settling is disabled or nothing has +// been observed to settle from. +func (s *Service) settleUntil(feedback FeedbackReport) *time.Time { + if s.cfg.SettleWindow <= 0 || feedback.LastEvidenceAt.IsZero() { + return nil + } + at := feedback.LastEvidenceAt.Add(s.cfg.SettleWindow).UTC() + return &at +} + +// advance moves the queue one step on this caller's behalf. +// +// The account-wide FIFO and the fire slot exist to serialize exactly one thing: +// the primary reviewer's metered review. A round that will not spend that quota +// is not a queue citizen, and letting one wait its turn is how PRs ended up +// parked for hours behind blocked rounds whose quota they were never going to +// touch β€” a free-plan repo whose primary only ever posts a walkthrough, or a +// round degraded to its co-reviewers while the account is rate-limited. +// +// So this PR's own round gets resolved directly whenever its work is quota-free, +// and only otherwise does the global pump run. The two conditions come from the +// observation already taken, which is what keeps the bypass free: attempting it +// unconditionally would re-observe this PR on every call for a case that applies +// to a minority of rounds. +// It reports whether it resolved this round through the quota-free path, which +// is how the caller knows a co-review was just set in motion and the schedule +// computed before it is now too long. +func (s *Service) advance(ctx context.Context, repo string, pr int, feedback FeedbackReport) (bool, error) { + if feedback.PrimaryUnavailable || feedback.CodeRabbitDeferred { + if _, handled, err := s.advanceQuotaFree(ctx, repo, pr); err != nil { + return false, err + } else if handled { + return true, nil + } + } + _, err := s.Pump(ctx) + return false, err +} + +// nextFromState derives the instruction from what is already recorded and +// observable. It writes NOTHING β€” no enqueue, no pump, no dashboard sync β€” which +// is what lets `crq wait` re-evaluate as often as it likes without spending the +// account's write budget or firing reviews behind the caller's back. +// +// ONE observation drives the whole decision. Feedback already reads the pull, so +// head, open, per-bot evidence and findings all describe the same instant. +// Reading the head separately used to let a push land between the two and answer +// "done" for a head nobody had reviewed. +// +// Both `crq next` and `crq wait` decide here, through the same pure +// engine.NextAction, so the blocking and non-blocking forms cannot disagree. +func (s *Service) nextFromState(ctx context.Context, repo string, pr int) (NextReport, engine.Action, FeedbackReport, error) { + report := NextReport{Repo: repo, PR: pr, Findings: []dialect.Finding{}, CheckedAt: s.clock()} + + feedback, err := s.Feedback(ctx, repo, pr) + if err != nil { + return report, engine.Action{}, feedback, err + } + report.Head = feedback.Head + report.ReviewedBy = feedback.ReviewedBy + + st, _, err := s.store.Load(ctx) + if err != nil { + return report, engine.Action{}, feedback, err + } + now := s.clock() + round := st.Round(repo, pr) + + report.LocalWork, report.LocalWorkReason = s.checkLocalWork(ctx, + []string{repo, feedback.HeadRepo}, report.Head, feedback.HeadRef) + + in := engine.NextInput{ + Obs: engine.Observation{Head: feedback.Head, Open: feedback.Open}, + Completion: engine.CompletionStatus{ReviewedBy: feedback.ReviewedBy, Done: allReviewed(feedback.ReviewedBy)}, + Findings: feedback.Findings, + Global: s.global(st, now), + Primary: s.cfg.Bot, + LocalWork: report.LocalWork, + Deferred: feedback.CodeRabbitDeferred, + DeferredUntil: feedback.DeferredUntil, + MinDelay: s.cfg.PollInterval, + SettleUntil: s.settleUntil(feedback), + } + // Only a round that still tracks THIS head may shape the verdict. A stale + // fired/reviewing round carries its own phase and deadline, and an elapsed + // one would report a terminal `blocked` for a head it never covered. + if round != nil && round.Head == feedback.Head { + in.Round = *round + } + + action := engine.NextAction(in, now) + report.Action = string(action.Kind) + report.Reason = action.Reason + report.Pending = action.Pending + if len(action.Findings) > 0 { + report.Findings = action.Findings + } + if !action.At.IsZero() { + at := action.At.UTC() + report.RecheckAfter = &at + } + return report, action, feedback, nil +} + +// NextWaiting is Next for an interactive caller: it sleeps through the states a +// caller cannot act on (wait, hold) and returns the first actionable +// instruction. It shares Next's code path exactly, so the blocking and +// non-blocking forms can never disagree about what should happen. +func (s *Service) NextWaiting(ctx context.Context, repo string, pr int) (NextReport, error) { + for { + report, err := s.Next(ctx, repo, pr) + if err != nil { + if wait, ok := ghapi.ThrottleWait(err); ok { + if wait <= 0 { + wait = s.cfg.PollInterval + } + if serr := ghapi.SleepCtx(ctx, wait); serr != nil { + return report, serr + } + continue + } + return report, err + } + switch engine.ActionKind(report.Action) { + case engine.ActionWait, engine.ActionHold: + if report.RecheckAfter == nil { + return report, nil + } + delay := report.RecheckAfter.Sub(s.clock()) + if delay <= 0 { + delay = s.cfg.PollInterval + } + if s.log != nil { + s.log.Printf("%s#%d %s β€” %s; rechecking at %s", + repo, pr, report.Action, report.Reason, report.RecheckAfter.Format(time.RFC3339)) + } + if serr := ghapi.SleepCtx(ctx, delay); serr != nil { + return report, serr + } + default: + return report, nil + } + } +} + +func (s *Service) checkLocalWork(ctx context.Context, repos []string, head, headRef string) (bool, string) { + if s.localWorkFn != nil { + return s.localWorkFn(ctx, head) + } + return localWork(ctx, repos, head, headRef) +} + +// localWork reports whether the working copy holds changes the PR head does not +// have. It is the difference between "push your fixes" and "nothing left to do", +// so both mistakes are expensive: a false negative strands finished work behind +// a terminal `done`, and a false positive tells the caller to push something +// that is not this PR at all. +// +// The order matters. It first checks the checkout belongs to one of the PR's +// repositories β€” the base, or on a fork PR the head repository, which is the +// only remote a contributor's clone has. Then it establishes the checkout is on +// the PR's line of history BEFORE reading a dirty tree as this PR's work: an +// unrelated branch of the right repository is dirty for its own reasons, and +// pushing it would land somebody else's changes. +// +// Anything it cannot establish answers false with a reason, which errs toward +// `done` rather than `push`. That is the safe direction: `push` is only ever +// emitted once the head is already released, so a missed one costs one extra +// call, while a spurious `hold` would stall the loop. +func localWork(ctx context.Context, repos []string, head, headRef string) (bool, string) { + git := func(args ...string) (string, bool) { + out, err := exec.CommandContext(ctx, "git", args...).Output() + if err != nil { + return "", false + } + return strings.TrimSpace(string(out)), true + } + if _, ok := git("rev-parse", "--is-inside-work-tree"); !ok { + return false, "not run inside a git checkout" + } + remotes, ok := git("remote", "-v") + if !ok { + return false, "could not read this checkout's remotes" + } + matched := "" + for _, candidate := range repos { + if candidate != "" && remoteMatchesRepo(remotes, candidate) { + matched = candidate + break + } + } + if matched == "" { + return false, "this checkout has no remote for " + strings.Join(nonEmpty(repos), " or ") + } + local, ok := git("rev-parse", "HEAD") + if !ok { + return false, "could not read local HEAD" + } + + // Sitting exactly on the PR head: only an uncommitted change is new work β€” + // but the caller still has to be able to push it. A detached checkout at the + // PR SHA has no branch, so the prescribed push fails and the next call, now + // ahead of the head, reports no local work and can converge over the fix. + if head == "" || strings.HasPrefix(local, head) { + if why := branchMismatch(git, headRef); why != "" { + return false, why + } + if status, ok := git("status", "--porcelain"); ok && status != "" { + return true, "uncommitted changes in the working tree" + } + return false, "" + } + + // Otherwise the checkout is somewhere else, and only ancestry can say + // whether that somewhere is this PR's branch carrying unpushed commits, a + // checkout merely behind it, or an unrelated branch entirely. + if _, known := git("rev-parse", "--verify", "--quiet", head+"^{commit}"); !known { + return false, "the pr head " + head + " is not in this checkout, so its relation to local HEAD is unknown" + } + if _, ahead := git("merge-base", "--is-ancestor", head, "HEAD"); !ahead { + return false, "local HEAD " + shortSHA(local) + " is not on the pr's branch (behind it, or a different branch)" + } + // Descending from the PR head is not the same as being the PR branch: a + // feature branch forked off it also descends, and reporting its commits as + // this PR's work invites a push that updates something else entirely. + if why := branchMismatch(git, headRef); why != "" { + return false, why + } + return true, "local HEAD " + shortSHA(local) + " is ahead of the pr head " + head +} + +// branchMismatch explains why this checkout is not the PR's branch, or "" when +// it is. A detached checkout counts as a mismatch: there is nothing to push to. +func branchMismatch(git func(...string) (string, bool), headRef string) string { + if headRef == "" { + return "" + } + branch, ok := git("rev-parse", "--abbrev-ref", "HEAD") + if !ok || branch == "HEAD" { + return "this checkout is detached, so there is no branch to push to " + headRef + } + if branch != headRef { + return "checked out " + branch + ", not the pr branch " + headRef + } + return "" +} + +// nonEmpty drops blanks so a reason never reads "owner/repo or ". +func nonEmpty(values []string) []string { + out := make([]string, 0, len(values)) + for _, v := range values { + if v != "" { + out = append(out, v) + } + } + return out +} + +// remoteMatchesRepo reports whether any configured remote points at repo. +// +// It compares the owner/name slug exactly. Substring-matching the raw +// `git remote -v` output made "owner/app" match a checkout of +// "owner/application", which then had its unrelated HEAD read as unlanded work. +func remoteMatchesRepo(remotes, repo string) bool { + want := strings.ToLower(strings.TrimSuffix(strings.TrimSpace(repo), ".git")) + if want == "" { + return false + } + for _, line := range strings.Split(remotes, "\n") { + fields := strings.Fields(line) + if len(fields) < 2 { + continue + } + if repoSlugFromRemote(fields[1]) == want { + return true + } + } + return false +} + +// repoSlugFromRemote reduces a git remote URL to its lowercase "owner/name", +// covering https, ssh:// and scp-style forms β€” including the host aliases +// (git@github.com-work:owner/name.git) a multi-account setup produces. +func repoSlugFromRemote(remote string) string { + url := strings.ToLower(strings.TrimSpace(remote)) + url = strings.TrimSuffix(url, ".git") + // scp-style separates the path with ":" rather than "/"; flattening both + // lets one segment walk handle every form. + url = strings.ReplaceAll(url, ":", "/") + var segments []string + for _, segment := range strings.Split(url, "/") { + if segment != "" { + segments = append(segments, segment) + } + } + if len(segments) < 2 { + return "" + } + return segments[len(segments)-2] + "/" + segments[len(segments)-1] +} + +func shortSHA(sha string) string { + if len(sha) > 9 { + return sha[:9] + } + return sha +} diff --git a/internal/crq/next_test.go b/internal/crq/next_test.go new file mode 100644 index 0000000..3599d7a --- /dev/null +++ b/internal/crq/next_test.go @@ -0,0 +1,304 @@ +package crq + +import ( + "context" + "fmt" + "strings" + "testing" + "time" + + "github.com/kristofferR/coderabbit-queue/internal/dialect" + "github.com/kristofferR/coderabbit-queue/internal/engine" + ghapi "github.com/kristofferR/coderabbit-queue/internal/gh" +) + +// setLocalWork pins Next's "does the caller hold unlanded changes" probe so the +// replay does not depend on the checkout the test happens to run in. +func (f *replayFixture) setLocalWork(has bool, reason string) { + f.svc.localWorkFn = func(context.Context, string) (bool, string) { return has, reason } +} + +func (f *replayFixture) closePull(repo string, pr int) { + f.gh.mu.Lock() + defer f.gh.mu.Unlock() + p := f.gh.pulls[fakeKey(repo, pr)] + p.State = "closed" + f.gh.pulls[fakeKey(repo, pr)] = p +} + +func (f *replayFixture) next(repo string, pr int) NextReport { + f.t.Helper() + report, err := f.svc.Next(f.ctx, repo, pr) + if err != nil { + f.t.Fatalf("next %s#%d: %v", repo, pr, err) + } + return report +} + +func (f *replayFixture) wantAction(report NextReport, want engine.ActionKind) { + f.t.Helper() + if report.Action != string(want) { + f.t.Fatalf("action = %q (%s), want %q", report.Action, report.Reason, want) + } +} + +// A whole review round driven only by `crq next`: the caller never chooses a +// delay, never decides when the head may move, and never reads an exit code. +// Each step asserts the instruction crq returns for a state an agent would +// otherwise have to reason about on its own. +func TestNextDrivesAReviewRound(t *testing.T) { + base := time.Date(2026, 7, 26, 9, 0, 0, 0, time.UTC) + f := newReplayFixture(t, base) + repo, pr := "owner/repo", 501 + head := "aaaaaaaa1" + f.openPull(repo, pr, head) + f.setCommitDate(head, base.Add(-time.Minute)) + f.setLocalWork(false, "") + + // 1. Nothing has happened yet: crq fires the review and tells the caller to + // wait β€” with a time it computed, not one the caller invented. + first := f.next(repo, pr) + f.wantAction(first, engine.ActionWait) + if first.RecheckAfter == nil { + t.Fatal("wait must carry a recheck time") + } + if !first.RecheckAfter.After(f.clk.now()) { + t.Errorf("recheck_after %s is not in the future", first.RecheckAfter) + } + if r := f.round(repo, pr); r == nil || r.FiredAt == nil { + t.Fatalf("next must advance the queue: round = %+v", r) + } + + // 2. The reviewer answers with a finding. The caller is told to fix it. + f.clk.advance(2 * time.Minute) + f.botReview(repo, pr, 900, head, f.clk.now()) + f.botReviewComment(repo, pr, 901, head, "internal/state/state.go", 42, + "_⚠️ Potential issue_\n\nThis dereferences a nil round.") + withFinding := f.next(repo, pr) + f.wantAction(withFinding, engine.ActionFix) + if len(withFinding.Findings) == 0 { + t.Fatal("fix must carry the findings to act on") + } + + // 3. The head moved and the caller still holds changes. No review has been + // requested for this head, so holding would stall for nobody β€” and + // queueing one now would spend a window on code about to be replaced. + // Land it first. + f.setLocalWork(true, "uncommitted changes in the working tree") + f.clk.advance(time.Minute) + f.setHead(repo, pr, "bbbbbbbb2") + f.setCommitDate("bbbbbbbb2", f.clk.now()) + postedBefore := f.reviewsPosted(repo, pr) + f.wantAction(f.next(repo, pr), engine.ActionPush) + if got := f.reviewsPosted(repo, pr); got != postedBefore { + t.Errorf("a head that is about to move must not buy a review: posted %d -> %d", postedBefore, got) + } + + // Once a review IS running for this head, the head must not move. + f.setLocalWork(false, "") + f.enqueue(repo, pr) + f.pump() + f.setLocalWork(true, "uncommitted changes in the working tree") + held := f.next(repo, pr) + if held.Action != string(engine.ActionHold) && held.Action != string(engine.ActionWait) { + t.Fatalf("action = %q (%s), want hold or wait while the review runs", held.Action, held.Reason) + } + if held.RecheckAfter == nil { + t.Error("a hold must say when to look again") + } + + // 4. The reviewer answers on the new head with nothing to say, and the + // caller has nothing left locally: converged. + f.setLocalWork(false, "") + f.clk.advance(time.Minute) + f.pump() + f.botReview(repo, pr, 902, "bbbbbbbb2", f.clk.now()) + done := f.next(repo, pr) + f.wantAction(done, engine.ActionDone) + if len(done.Findings) != 0 { + t.Errorf("done must carry no findings, got %d", len(done.Findings)) + } +} + +// `next` advances the queue as a side effect, and that step owns the review +// fire β€” so the two halves of drain-first are both properties of ONE decision: +// undrained feedback for the current head must not buy another review of that +// same head, and feedback carried from an older commit must not stop the new +// head from being reviewed at all. +func TestNextDrainsCurrentHeadWithoutDeadlockingTheNextOne(t *testing.T) { + base := time.Date(2026, 7, 26, 9, 0, 0, 0, time.UTC) + f := newReplayFixture(t, base) + repo, pr := "owner/repo", 504 + head := "aaaaaaaa1" + f.openPull(repo, pr, head) + f.setCommitDate(head, base.Add(-time.Minute)) + f.setLocalWork(false, "") + + f.next(repo, pr) + if got := f.reviewsPosted(repo, pr); got != 1 { + t.Fatalf("the first call must fire exactly one review, got %d", got) + } + + // Feedback lands ON this head. Until the caller drains it, another review of + // the same head would spend account quota to be told the same thing. + f.clk.advance(2 * time.Minute) + f.botReview(repo, pr, 900, head, f.clk.now()) + f.botReviewComment(repo, pr, 901, head, "internal/state/state.go", 42, + "_⚠️ Potential issue_\n\nThis dereferences a nil round.") + f.wantAction(f.next(repo, pr), engine.ActionFix) + if got := f.reviewsPosted(repo, pr); got != 1 { + t.Fatalf("undrained feedback for this head must not buy another review, posted %d", got) + } + + // The caller pushes a fix. The old finding belongs to the previous commit, + // so it can no longer be acted on here β€” and must not keep the new head from + // being reviewed. This is the deadlock the narrow gate exists to avoid. + f.clk.advance(time.Minute) + f.setHead(repo, pr, "bbbbbbbb2") + f.setCommitDate("bbbbbbbb2", f.clk.now()) + f.next(repo, pr) + if got := f.reviewsPosted(repo, pr); got != 2 { + t.Fatalf("the new head must still get its own review, posted %d", got) + } +} + +// A closed PR is not something a loop can resolve by waiting. +func TestNextBlocksOnClosedPR(t *testing.T) { + base := time.Date(2026, 7, 26, 9, 0, 0, 0, time.UTC) + f := newReplayFixture(t, base) + repo, pr := "owner/repo", 502 + f.openPull(repo, pr, "aaaaaaaa1") + f.setCommitDate("aaaaaaaa1", base.Add(-time.Minute)) + f.setLocalWork(false, "") + f.next(repo, pr) + + f.closePull(repo, pr) + f.wantAction(f.next(repo, pr), engine.ActionBlocked) +} + +// Every wait crq hands back is at least a poll interval away, so a caller +// looping on `crq next` cannot spin β€” the floor is arithmetic, not etiquette. +func TestNextNeverSchedulesAHotLoop(t *testing.T) { + base := time.Date(2026, 7, 26, 9, 0, 0, 0, time.UTC) + f := newReplayFixture(t, base) + repo, pr := "owner/repo", 503 + f.openPull(repo, pr, "aaaaaaaa1") + f.setCommitDate("aaaaaaaa1", base.Add(-time.Minute)) + f.setLocalWork(false, "") + + for i := 0; i < 3; i++ { + report := f.next(repo, pr) + if report.RecheckAfter == nil { + continue + } + if got := report.RecheckAfter.Sub(f.clk.now()); got < f.cfg.PollInterval { + t.Fatalf("recheck in %s, want at least the poll interval %s", got, f.cfg.PollInterval) + } + } +} + +// The local-work probe decides push-vs-done, so "is this checkout even the +// PR's repository" has to be exact. Substring matching made owner/app match a +// checkout of owner/application and read its HEAD as unlanded work. +func TestRemoteMatchesRepo(t *testing.T) { + remotes := func(urls ...string) string { + var b strings.Builder + for i, u := range urls { + fmt.Fprintf(&b, "r%d\t%s (fetch)\nr%d\t%s (push)\n", i, u, i, u) + } + return b.String() + } + cases := []struct { + name string + remotes string + repo string + want bool + }{ + {"https", remotes("https://github.com/owner/app.git"), "owner/app", true}, + {"scp form", remotes("git@github.com:owner/app.git"), "owner/app", true}, + {"ssh url", remotes("ssh://git@github.com/owner/app"), "owner/app", true}, + {"host alias for a second account", remotes("git@github.com-work:owner/app.git"), "owner/app", true}, + {"case insensitive", remotes("https://github.com/Owner/App.git"), "owner/app", true}, + {"fork checkout with the upstream as a second remote", + remotes("git@github.com:me/app.git", "https://github.com/owner/app.git"), "owner/app", true}, + {"prefix of a longer name does not match", + remotes("https://github.com/owner/application.git"), "owner/app", false}, + {"same name under another owner does not match", + remotes("https://github.com/other/app.git"), "owner/app", false}, + {"no remotes", "", "owner/app", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := remoteMatchesRepo(tc.remotes, tc.repo); got != tc.want { + t.Errorf("remoteMatchesRepo(%q) = %v, want %v", tc.repo, got, tc.want) + } + }) + } +} + +// The queue exists to serialize ONE thing: CodeRabbit's account-wide review +// limit. A round that will never spend that quota is not a queue citizen, so +// neither an account block nor another PR holding the fire slot may delay it. +// +// `crq loop` learned this; `crq next` did not, and inherited the starvation the +// dogfood exposed β€” summary-only rounds sitting behind blocked rounds they would +// never spend quota on. Staged at its worst here: another PR HOLDS the slot, so +// Pump short-circuits on the slot holder and never reaches the FIFO or its +// bounded rescue scan at all. +func TestNextResolvesSummaryOnlyWithoutTheQueue(t *testing.T) { + cfg := firingConfig() + cfg.RequiredBots = []string{"coderabbitai[bot]", dialect.CodexBotLogin} + cfg.CoBots = codexCoBots(cfg.RequiredBots) + cfg.FeedbackBots = cfg.RequiredBots + gh := newFakeGitHub() + gh.graphQL = noForcePush + now := time.Now().UTC() + + frontPull := ghapi.Pull{State: "open"} + frontPull.Head.SHA = "1111111111111111" + gh.pulls[fakeKey("o/front", 10)] = frontPull + + pull := ghapi.Pull{State: "open"} + pull.Head.SHA = "2222222222222222" + gh.pulls[fakeKey("o/private", 20)] = pull + walkthrough := ghapi.IssueComment{ + ID: 900, + Body: corpusMessage(t, "coderabbit/summary-only-free-plan.md"), + CreatedAt: now.Add(-time.Minute), + UpdatedAt: now.Add(-time.Minute), + } + walkthrough.User.Login = "coderabbitai[bot]" + gh.comments[fakeKey("o/private", 20)] = []ghapi.IssueComment{walkthrough} + + store := NewMemoryStore(cfg) + svc := NewService(cfg, gh, store, nil) + svc.now = func() time.Time { return now } + svc.localWorkFn = func(context.Context, string) (bool, string) { return false, "" } + + seedRound(t, store, cfg, "o/front", 10, "111111111", PhaseFired, now.Add(-time.Minute), 500) + blockedUntil := now.Add(45 * time.Minute) + if _, err := store.Update(context.Background(), func(st *State) error { + st.Account.BlockedUntil = &blockedUntil + st.FireSlot = &FireSlot{Key: QueueKey("o/front", 10), Token: "seedtok", Since: now.Add(-time.Minute)} + return nil + }); err != nil { + t.Fatal(err) + } + + if _, err := svc.Next(context.Background(), "o/private", 20); err != nil { + t.Fatalf("next: %v", err) + } + + st, _, _ := store.Load(context.Background()) + if r := st.Round("o/private", 20); r == nil || r.Phase == PhaseQueued { + t.Fatalf("the summary-only round must leave the queue, got %#v", r) + } + for _, p := range gh.posted { + if strings.Contains(p, cfg.ReviewCommand) { + t.Fatalf("summary-only must never post the CodeRabbit command, posted=%v", gh.posted) + } + } + if st.FireSlot == nil || st.FireSlot.Key != QueueKey("o/front", 10) { + t.Fatalf("the front PR must keep the fire slot, got %#v", st.FireSlot) + } +} diff --git a/internal/crq/preflight.go b/internal/crq/preflight.go index cbbb3f7..70f27fa 100644 --- a/internal/crq/preflight.go +++ b/internal/crq/preflight.go @@ -40,9 +40,17 @@ type PreflightReport struct { Findings []PreflightFinding `json:"findings"` Stderr string `json:"stderr,omitempty"` Error string `json:"error,omitempty"` - ExitCode int `json:"exit_code"` - CheckedAt time.Time `json:"checked_at"` - DurationMS int64 `json:"duration_ms"` + // ErrorType is the CLI's own classification of a failure ("rate_limit", + // ...). Recoverable and RetryAfter come with it. crq keeps them because the + // local CLI shares the SAME account quota as the PR reviews the queue + // serializes: a local rate limit is direct evidence about that quota, given + // for free, with no probe comment and no GitHub round trip. + ErrorType string `json:"error_type,omitempty"` + Recoverable bool `json:"recoverable,omitempty"` + RetryAfter string `json:"retry_after,omitempty"` + ExitCode int `json:"exit_code"` + CheckedAt time.Time `json:"checked_at"` + DurationMS int64 `json:"duration_ms"` } type PreflightStatus struct { @@ -149,7 +157,18 @@ func Preflight(ctx context.Context, opts PreflightOptions) (PreflightReport, int code = exitErr.ExitCode() } report.Status = "error" - report.Error = waitErr.Error() + // Keep the CLI's own message. Overwriting it with "exit status 1" threw + // away the only useful part β€” a caller was told the command failed but + // not that the account was rate-limited, nor for how long. + if report.Error == "" { + report.Error = waitErr.Error() + } + // A rate limit is not a broken setup: nothing is misconfigured and the + // answer is to come back later, which is what exit 2 already means here. + if dialect.IsCLIRateLimit(report.ErrorType) { + report.Status = "rate_limited" + code = 2 + } report.ExitCode = code return report, code, waitErr } @@ -235,6 +254,13 @@ func applyPreflightEvent(report *PreflightReport, event map[string]any) { if msg := firstNonEmpty(stringField(event, "message"), stringField(event, "error")); msg != "" { report.Error = msg } + report.ErrorType = stringField(event, "errorType") + if v, ok := event["recoverable"].(bool); ok { + report.Recoverable = v + } + if meta, ok := event["metadata"].(map[string]any); ok { + report.RetryAfter = stringField(meta, "waitTime") + } } } diff --git a/internal/crq/service.go b/internal/crq/service.go index bfcddcb..287cebd 100644 --- a/internal/crq/service.go +++ b/internal/crq/service.go @@ -37,6 +37,10 @@ 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 + // 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. + GetRef(context.Context, string, string) (string, error) } type Service struct { @@ -57,6 +61,14 @@ type Service struct { // deterministically. It intentionally does NOT reach logging/jitter/token or // the fake GitHub timestamps, which stay on real time. now func() time.Time + // localWorkFn overrides Next's "does the caller hold changes the PR head + // lacks" probe, which otherwise shells out to git in the process's working + // directory. nil in production; tests inject an answer instead of depending + // on the checkout they happen to run in. + localWorkFn func(ctx context.Context, head string) (bool, string) + // sleepFn overrides how the waiter idles. Only tests set it β€” a replay must + // not spend real seconds to prove what the injected clock already decides. + sleepFn func(ctx context.Context, d time.Duration) error } func NewService(cfg Config, gh GitHubAPI, store StateStore, log Logger) *Service { @@ -104,6 +116,10 @@ func (s *Service) Enqueue(ctx context.Context, repo string, pr int) (EnqueueResu if err != nil { return result, err } + // Always report the head this actually read. It used to be set only on the + // deduped path, so a caller comparing it against its own observation could + // not detect the very case that matters β€” a head that moved between the two. + result.Head = head state, err := s.store.Update(ctx, func(st *State) error { now := s.clock() r := st.Round(repo, pr) diff --git a/internal/crq/service_test.go b/internal/crq/service_test.go index 0bc24a7..19c4241 100644 --- a/internal/crq/service_test.go +++ b/internal/crq/service_test.go @@ -35,7 +35,11 @@ type fakeGitHub struct { nextIssueNumber int postErrs map[string]error graphQL func(query string, vars map[string]any, out any) error - searchPRs []ghapi.SearchPR + // stateRef is the SHA GetRef reports; tests that exercise `crq wait` move it + // to signal "the queue advanced". + stateRef string + refReads int + searchPRs []ghapi.SearchPR // now, when set, timestamps posted comments off the same injected clock the // service uses, so a fire's recorded FiredAt tracks the fake wall clock the // replay suite advances. nil falls back to real time (all existing tests). @@ -231,6 +235,24 @@ func (f *fakeGitHub) EachOpenPR(_ context.Context, _ string, _ bool, fn func(gha return 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) { + f.mu.Lock() + defer f.mu.Unlock() + f.refReads++ + if f.stateRef == "" { + return "ref0", nil + } + return f.stateRef, nil +} + +func (f *fakeGitHub) setStateRef(sha string) { + f.mu.Lock() + defer f.mu.Unlock() + f.stateRef = sha +} + func (f *fakeGitHub) GraphQL(_ context.Context, query string, vars map[string]any, out any) error { f.mu.Lock() handler := f.graphQL diff --git a/internal/crq/wait.go b/internal/crq/wait.go new file mode 100644 index 0000000..b756270 --- /dev/null +++ b/internal/crq/wait.go @@ -0,0 +1,208 @@ +package crq + +import ( + "context" + "time" + + "github.com/kristofferR/coderabbit-queue/internal/engine" + ghapi "github.com/kristofferR/coderabbit-queue/internal/gh" +) + +// maxStaleFactor bounds how long the waiter will trust the state ref alone. +// +// The ref is a necessary but not sufficient wake signal: a bot posting a review +// does not move it β€” only a daemon observing that review and transitioning the +// round does. So the waiter re-evaluates on a ceiling too, set from the leader +// lease it already has in hand. Two lease periods means a daemon has had two +// full chances to notice before the waiter looks for itself. +const maxStaleFactor = 2 + +// minWaitTick floors the idle cadence. +// +// Every interval the waiter derives comes from config, and config can be zero: +// an unset CRQ_LEADER_TTL puts the staleness ceiling in the past, and an unset +// CRQ_POLL makes the tick zero. Either turns this loop into a hot spin against +// the shared API budget β€” the exact failure the command exists to prevent β€” so +// the floor is arithmetic here rather than a documented expectation of config. +const minWaitTick = time.Second + +// waitTick is the idle poll cadence, never zero. +func (s *Service) waitTick() time.Duration { + if s.cfg.PollInterval > minWaitTick { + return s.cfg.PollInterval + } + return minWaitTick +} + +// WaitForAction blocks until there is something for the caller to DO, then +// prints that instruction and exits. +// +// It exists because the wait, not the decision, was the part agents could not +// hold. `crq loop` blocked AND owned the round, so a harness killing it between +// turns either re-fired a review on restart (spending account quota) or hit the +// dedupe and reported a converged round it never collected findings for. Both +// branches are wrong, and every observed session wrapped the command in +// `set +e … ; echo "CRQ_EXIT:$?"` to smuggle the exit code back out. +// +// This owns nothing: it holds no round and has no exit-code vocabulary of its +// own, so killing it costs exactly the process. Its ONLY job is to notice, so +// that its exit can be the wake event for an agent that ended its turn. If it +// dies, the caller re-runs it (or calls `crq next`) and gets the same answer +// from persisted state. +// +// It is read-only in the steady state, but NOT unconditionally: when nothing is +// advancing this PR β€” no round for the head, or no live leader β€” it drives the +// queue itself through Next, which enqueues and can post the review command. +// The alternative is idling forever on a queue nobody will put the PR in, so the +// honest contract is "writes only when it must, to avoid waiting for nobody". +// +// Cost matters as much as correctness here: the account shares one REST budget +// across the daemon and every agent, and seven concurrent waiters once +// out-spent it on their own. So the loop watches the state ref with a +// conditional GET β€” an unchanged ref answers 304 and costs no quota β€” and only +// pays for a full evaluation when the ref moves or the staleness ceiling +// elapses. +func (s *Service) WaitForAction(ctx context.Context, repo string, pr int) (NextReport, error) { + repo = NormalizeRepo(repo) + for { + // Sample the ref BEFORE deciding. Taken after, a daemon transition landing + // between the decision and the first read would be recorded as the + // baseline instead of recognised as a change, and the waiter would sleep + // to its ceiling β€” up to two leader periods β€” with an actionable answer + // already sitting there. + lastRef, refErr := s.gh.GetRef(ctx, s.cfg.GateRepo, s.cfg.StateRef) + + report, action, _, err := s.nextFromState(ctx, repo, pr) + if err != nil { + if wait, throttled := ghapi.ThrottleWait(err); throttled { + if wait <= 0 { + wait = s.waitTick() + } + if serr := s.sleep(ctx, wait); serr != nil { + return report, serr + } + continue + } + return report, err + } + if actionable(action.Kind) { + return report, nil + } + + // Nothing to do yet. Someone has to be advancing the queue, or this waits + // forever: the daemon normally does, and a live leader lease is how the + // waiter knows. Without one it drives the queue itself through Next, + // which enqueues and pumps β€” correct, just costlier. + // + // A live leader is not enough on its own. The daemon only advances PRs in + // its own scope, so a PR outside the fleet with no round for this head + // would wait forever on a queue nobody was ever going to put it in. If + // this head is untracked, drive it regardless of who holds the lease. + st, _, err := s.store.Load(ctx) + if err != nil { + // The same shared-quota pressure this command exists to survive: the + // first load sleeps through throttling, so this one must too, or the + // waiter exits precisely when the fleet is busiest. + if wait, throttled := ghapi.ThrottleWait(err); throttled { + if wait <= 0 { + wait = s.waitTick() + } + if serr := s.sleep(ctx, wait); serr != nil { + return report, serr + } + continue + } + return report, err + } + now := s.clock() + round := st.Round(repo, pr) + untracked := round == nil || (report.Head != "" && round.Head != report.Head) + if untracked || !leaderLive(st, now) { + if _, nerr := s.Next(ctx, repo, pr); nerr != nil { + return report, nerr + } + if serr := s.sleep(ctx, s.waitTick()); serr != nil { + return report, serr + } + continue + } + + deadline := now.Add(maxStaleFactor * s.cfg.LeaderTTL) + if report.RecheckAfter != nil && report.RecheckAfter.Before(deadline) { + deadline = *report.RecheckAfter + } + // Never a deadline that has already passed: that would return from the + // watch without idling and spin this loop. + if floor := now.Add(s.waitTick()); deadline.Before(floor) { + deadline = floor + } + // Without a baseline the watch cannot recognise a change β€” it would adopt + // whatever it reads first β€” so a failed sample must not buy a long sleep. + // Come back after one tick and take the baseline again instead. + if refErr != nil { + deadline = now.Add(s.waitTick()) + } + if _, _, err := s.watchStateRef(ctx, lastRef, deadline); err != nil { + return report, err + } + } +} + +// sleep idles for d, honouring cancellation. Tests replace it so a replay costs +// no wall-clock time. +func (s *Service) sleep(ctx context.Context, d time.Duration) error { + if s.sleepFn != nil { + return s.sleepFn(ctx, d) + } + return ghapi.SleepCtx(ctx, d) +} + +// actionable reports whether an action is something the caller can act on now. +// wait and hold are the two the caller cannot: both mean "come back later", +// which is precisely what this command does on their behalf. +func actionable(kind engine.ActionKind) bool { + switch kind { + case engine.ActionWait, engine.ActionHold: + return false + default: + return true + } +} + +// leaderLive reports whether an autoreview daemon currently holds the lease and +// is therefore advancing rounds (and moving the state ref) on its own. +func leaderLive(st State, now time.Time) bool { + return st.Leader != nil && st.Leader.ExpiresAt.After(now) +} + +// watchStateRef polls the state ref until its SHA changes or deadline passes, +// returning the SHA last seen. +// +// This is the cheap half of the waiter: one conditional GET per tick, which +// costs no quota while the ref is unchanged. Every meaningful queue transition +// β€” fired, reviewing, completed, superseded β€” is a state write, so the ref +// moving is the signal that something worth re-deciding happened. +// +// A read failure is not fatal. Losing a tick only delays a re-evaluation the +// deadline would force anyway, and a waiter that exits on a transient GitHub +// error is exactly the fragility this command replaces. +func (s *Service) watchStateRef(ctx context.Context, lastRef string, deadline time.Time) (bool, string, error) { + for { + ref, err := s.gh.GetRef(ctx, s.cfg.GateRepo, s.cfg.StateRef) + if err == nil { + if lastRef != "" && ref != lastRef { + return true, ref, nil + } + lastRef = ref + } + tick := s.waitTick() + if remaining := deadline.Sub(s.clock()); remaining <= 0 { + return false, lastRef, nil + } else if remaining < tick { + tick = remaining + } + if serr := s.sleep(ctx, tick); serr != nil { + return false, lastRef, serr + } + } +} diff --git a/internal/crq/wait_test.go b/internal/crq/wait_test.go new file mode 100644 index 0000000..8211c4a --- /dev/null +++ b/internal/crq/wait_test.go @@ -0,0 +1,248 @@ +package crq + +import ( + "context" + "sync" + "testing" + "time" + + "github.com/kristofferR/coderabbit-queue/internal/engine" +) + +// leader marks an autoreview daemon as live, which is what tells the waiter +// somebody else is advancing the queue and it may idle on the state ref. +func (f *replayFixture) leader(until time.Time) { + f.t.Helper() + if _, err := f.svc.store.Update(f.ctx, func(st *State) error { + st.Leader = &LeaderLease{Owner: "daemon", Token: "t", ExpiresAt: until, UpdatedAt: f.clk.now()} + return nil + }); err != nil { + f.t.Fatalf("set leader: %v", err) + } +} + +func (f *replayFixture) writeCount() int { + f.gh.mu.Lock() + defer f.gh.mu.Unlock() + return len(f.gh.posted) + len(f.gh.deleted) + len(f.gh.createdIssues) +} + +// The waiter's whole value is that it can be killed. It must therefore hold no +// state and write nothing β€” so a harness that SIGTERMs it between turns leaves +// the round exactly as it was, and re-running it (or crq next) is correct. +// +// The observed failures were the opposite: killing `crq loop` either re-fired a +// review on restart, spending account quota, or hit the dedupe and reported a +// converged round whose findings were never collected. +func TestWaitOwnsNothingAndWritesNothing(t *testing.T) { + base := time.Date(2026, 7, 26, 9, 0, 0, 0, time.UTC) + f := newReplayFixture(t, base) + repo, pr := "owner/repo", 601 + head := "aaaaaaaa1" + f.openPull(repo, pr, head) + f.setCommitDate(head, base.Add(-time.Minute)) + f.setLocalWork(false, "") + + // The round is already under review, so the waiter has nothing to return yet. + f.next(repo, pr) + f.leader(f.clk.now().Add(time.Hour)) + before := f.writeCount() + roundBefore := *f.round(repo, pr) + + // Idle deterministically: block in the sleep until the context dies, which + // is exactly the state a harness SIGTERMs the waiter in. + idling := make(chan struct{}) + var once sync.Once + f.svc.sleepFn = func(ctx context.Context, _ time.Duration) error { + once.Do(func() { close(idling) }) + <-ctx.Done() + return ctx.Err() + } + + // Kill it mid-wait, exactly as a harness does at a turn boundary. + ctx, cancel := context.WithCancel(f.ctx) + done := make(chan struct{}) + go func() { + defer close(done) + f.svc.WaitForAction(ctx, repo, pr) //nolint:errcheck // cancellation is the point + }() + <-idling + cancel() + <-done + + if got := f.writeCount(); got != before { + t.Errorf("the wait wrote to GitHub (%d -> %d); it must own nothing", before, got) + } + after := f.round(repo, pr) + if after == nil || after.Phase != roundBefore.Phase || after.Head != roundBefore.Head || after.Seq != roundBefore.Seq { + t.Errorf("the round changed across a killed wait: %+v -> %+v", roundBefore, after) + } + + // And the killed wait cost nothing: the answer is still there for the asking. + f.svc.sleepFn = nil + f.wantAction(f.next(repo, pr), engine.ActionWait) +} + +// The waiter returns the moment there is something to act on, and returns the +// same instruction crq next would β€” they share one decision function precisely +// so the blocking and non-blocking forms cannot disagree. +func TestWaitReturnsWhenTheRoundBecomesActionable(t *testing.T) { + base := time.Date(2026, 7, 26, 9, 0, 0, 0, time.UTC) + f := newReplayFixture(t, base) + repo, pr := "owner/repo", 602 + head := "aaaaaaaa1" + f.openPull(repo, pr, head) + f.setCommitDate(head, base.Add(-time.Minute)) + f.setLocalWork(false, "") + + f.next(repo, pr) + f.leader(f.clk.now().Add(time.Hour)) + + // Feedback lands before the wait starts, so it returns on its first pass + // rather than idling β€” the cheap path is an optimization, not a delay. + f.clk.advance(2 * time.Minute) + f.botReview(repo, pr, 900, head, f.clk.now()) + f.botReviewComment(repo, pr, 901, head, "internal/state/state.go", 42, + "_⚠️ Potential issue_\n\nThis dereferences a nil round.") + + report, err := f.svc.WaitForAction(f.ctx, repo, pr) + if err != nil { + t.Fatalf("wait: %v", err) + } + if report.Action != string(engine.ActionFix) { + t.Fatalf("action = %q (%s), want %q", report.Action, report.Reason, engine.ActionFix) + } + if len(report.Findings) == 0 { + t.Error("an actionable return must carry the findings to act on") + } +} + +// A converged round is actionable too: `done` is an answer, not a state to keep +// waiting through. Without this the waiter would idle forever on a finished PR. +func TestWaitReturnsOnConvergence(t *testing.T) { + base := time.Date(2026, 7, 26, 9, 0, 0, 0, time.UTC) + f := newReplayFixture(t, base) + repo, pr := "owner/repo", 603 + head := "aaaaaaaa1" + f.openPull(repo, pr, head) + f.setCommitDate(head, base.Add(-time.Minute)) + f.setLocalWork(false, "") + + f.next(repo, pr) + f.clk.advance(time.Minute) + f.botReview(repo, pr, 900, head, f.clk.now()) + + report, err := f.svc.WaitForAction(f.ctx, repo, pr) + if err != nil { + t.Fatalf("wait: %v", err) + } + if report.Action != string(engine.ActionDone) { + t.Fatalf("action = %q (%s), want %q", report.Action, report.Reason, engine.ActionDone) + } +} + +// actionable is the whole return condition, so state it as a table: the two +// states a caller cannot act on are exactly the two the waiter absorbs. +func TestActionableStates(t *testing.T) { + for kind, want := range map[engine.ActionKind]bool{ + engine.ActionFix: true, + engine.ActionPush: true, + engine.ActionDone: true, + engine.ActionBlocked: true, + engine.ActionWait: false, + engine.ActionHold: false, + } { + if got := actionable(kind); got != want { + t.Errorf("actionable(%q) = %v, want %v", kind, got, want) + } + } +} + +// Every interval the waiter idles on comes from config, and config can be zero. +// An unset LeaderTTL once put the staleness ceiling in the past, so the watch +// returned without idling and the outer loop spun against the API β€” the hot +// loop this command exists to prevent. The floor must be arithmetic. +func TestWaitNeverHotLoopsOnZeroedIntervals(t *testing.T) { + base := time.Date(2026, 7, 26, 9, 0, 0, 0, time.UTC) + f := newReplayFixture(t, base) + repo, pr := "owner/repo", 604 + head := "aaaaaaaa1" + f.openPull(repo, pr, head) + f.setCommitDate(head, base.Add(-time.Minute)) + f.setLocalWork(false, "") + + f.next(repo, pr) + f.leader(f.clk.now().Add(time.Hour)) + + // The pathological config: no cadence and no lease period to derive one from. + f.svc.cfg.PollInterval = 0 + f.svc.cfg.LeaderTTL = 0 + + ctx, cancel := context.WithCancel(f.ctx) + defer cancel() + var mu sync.Mutex + var slept []time.Duration + f.svc.sleepFn = func(_ context.Context, d time.Duration) error { + mu.Lock() + slept = append(slept, d) + n := len(slept) + mu.Unlock() + f.clk.advance(d) + if n >= 3 { + cancel() + return context.Canceled + } + return nil + } + + if _, err := f.svc.WaitForAction(ctx, repo, pr); err == nil { + t.Fatal("expected the cancelled wait to return an error") + } + mu.Lock() + defer mu.Unlock() + if len(slept) == 0 { + t.Fatal("the waiter spun without ever idling") + } + for i, d := range slept { + if d < minWaitTick { + t.Errorf("sleep %d was %s, want at least the %s floor", i, d, minWaitTick) + } + } +} + +// A live leader lease means "a daemon is advancing the queue", not "a daemon is +// advancing YOUR pr". The daemon only scans its own scope, so a PR outside the +// fleet with no round for this head would idle forever on a queue nobody was +// ever going to put it in. An untracked head drives itself regardless of who +// holds the lease. +func TestWaitDrivesAnUntrackedHeadEvenUnderALiveLeader(t *testing.T) { + base := time.Date(2026, 7, 26, 9, 0, 0, 0, time.UTC) + f := newReplayFixture(t, base) + repo, pr := "owner/outside-the-fleet", 701 + head := "aaaaaaaa1" + f.openPull(repo, pr, head) + f.setCommitDate(head, base.Add(-time.Minute)) + f.setLocalWork(false, "") + + // A daemon holds the lease, but has never heard of this PR. + f.leader(f.clk.now().Add(time.Hour)) + if r := f.round(repo, pr); r != nil { + t.Fatalf("precondition: no round should exist yet, got %#v", r) + } + + // Idle exactly once so the test cannot hang if the fix regresses. + ctx, cancel := context.WithCancel(f.ctx) + defer cancel() + f.svc.sleepFn = func(context.Context, time.Duration) error { + cancel() + return context.Canceled + } + f.svc.WaitForAction(ctx, repo, pr) //nolint:errcheck // the cancel is the bound + + if r := f.round(repo, pr); r == nil { + t.Fatal("the waiter must enqueue an untracked head instead of waiting on a daemon that cannot see it") + } + if got := f.reviewsPosted(repo, pr); got != 1 { + t.Errorf("the untracked head must get its review requested, posted %d", got) + } +} diff --git a/internal/dialect/coderabbit.go b/internal/dialect/coderabbit.go index c40b1af..724f562 100644 --- a/internal/dialect/coderabbit.go +++ b/internal/dialect/coderabbit.go @@ -399,3 +399,19 @@ func ParsePromptReviewFindings(body string, review ReviewMeta, bot string) []Fin } return out } + +// CLIRateLimitErrorType is the local CodeRabbit CLI's own name for an account +// quota block, as emitted on its --agent JSON stream. +// +// The CLI spends the SAME account quota as the PR reviews crq queues, so this +// string is quota evidence β€” but it is still CodeRabbit's vocabulary, and this +// package is the only place allowed to know it. Orchestration asks +// IsCLIRateLimit instead of matching the literal, so a renamed or added error +// type is a one-line change here plus a corpus row, not a silent fallback to a +// generic failure. +const CLIRateLimitErrorType = "rate_limit" + +// IsCLIRateLimit reports whether a CLI error type means the account is blocked. +func IsCLIRateLimit(errorType string) bool { + return strings.EqualFold(strings.TrimSpace(errorType), CLIRateLimitErrorType) +} diff --git a/internal/dialect/golden_test.go b/internal/dialect/golden_test.go index 7360b87..0a3b6c3 100644 --- a/internal/dialect/golden_test.go +++ b/internal/dialect/golden_test.go @@ -560,3 +560,34 @@ func TestAutoReviewsDisabledIsNotARefusal(t *testing.T) { t.Fatal("a real refusal must still be recognised") } } + +// The CLI's error vocabulary is CodeRabbit's, so it lives here with a verbatim +// fixture like every other wording crq depends on. The fixture is the real +// --agent stream event captured from a blocked account. +func TestGoldenCLIRateLimit(t *testing.T) { + raw, err := os.ReadFile(filepath.Join("testdata", "coderabbit", "cli-rate-limit.json")) + if err != nil { + t.Fatal(err) + } + var event struct { + ErrorType string `json:"errorType"` + Recoverable bool `json:"recoverable"` + Metadata struct { + WaitTime string `json:"waitTime"` + } `json:"metadata"` + } + if err := json.Unmarshal(raw, &event); err != nil { + t.Fatal(err) + } + if !IsCLIRateLimit(event.ErrorType) { + t.Errorf("IsCLIRateLimit(%q) = false, want true", event.ErrorType) + } + if !event.Recoverable || event.Metadata.WaitTime == "" { + t.Errorf("the fixture must carry the recoverable flag and a wait time: %+v", event) + } + for _, other := range []string{"", "auth", "network", "rate_limits"} { + if IsCLIRateLimit(other) { + t.Errorf("IsCLIRateLimit(%q) = true, want false", other) + } + } +} diff --git a/internal/dialect/testdata/coderabbit/cli-rate-limit.json b/internal/dialect/testdata/coderabbit/cli-rate-limit.json new file mode 100644 index 0000000..b56ba59 --- /dev/null +++ b/internal/dialect/testdata/coderabbit/cli-rate-limit.json @@ -0,0 +1 @@ +{"type":"error","errorType":"rate_limit","message":"Rate limit exceeded","recoverable":true,"details":{},"metadata":{"isProUser":false,"waitTime":"32 minutes","policyGuidance":"Enable **[usage-based reviews](https://app.coderabbit.ai/settings/billing?tab=usage)** in Billing to review now. Otherwise, wait until the next included review is available.\nYou're only billed for reviews past your plan's rate limits ($0.25/file).","orgAttributed":true,"cliReviewPolicyMode":"normal"}} diff --git a/internal/engine/next.go b/internal/engine/next.go new file mode 100644 index 0000000..328a236 --- /dev/null +++ b/internal/engine/next.go @@ -0,0 +1,334 @@ +package engine + +import ( + "sort" + "time" + + "github.com/kristofferR/coderabbit-queue/internal/dialect" + "github.com/kristofferR/coderabbit-queue/internal/state" +) + +// ActionKind is the single instruction a caller of `crq next` executes. It is a +// CLOSED set: an agent driving a review loop reads exactly this field and does +// exactly what it says, so every judgement call that used to be improvised β€” +// how long to sleep, whether the head may move, whether the round is finished β€” +// is answered here instead of at the call site. +type ActionKind string + +const ( + // ActionFix: actionable findings exist for this head. Fix them, validate, + // then resolve or decline each thread. + ActionFix ActionKind = "fix" + // ActionHold: the caller has work to land but a required reviewer has not + // answered for this head. Moving the head now would restart that review, so + // hold it and re-check at Action.At. + ActionHold ActionKind = "hold" + // ActionPush: the head is released β€” commit and push the accumulated fixes. + ActionPush ActionKind = "push" + // ActionWait: nothing to do until Action.At. + ActionWait ActionKind = "wait" + // ActionDone: every required reviewer answered and no findings remain. + ActionDone ActionKind = "done" + // ActionBlocked: the loop cannot proceed without a human (PR closed). + ActionBlocked ActionKind = "blocked" +) + +// Action is the answer NextAction produces. +type Action struct { + Kind ActionKind + Reason string + // At is when the caller should call again; set for hold and wait, and + // always strictly in the future (see NextInput.MinDelay). + At time.Time + // Pending lists the required bots with no review evidence for this head, + // in the caller's configured order. + Pending []string + // Findings carries the actionable feedback for ActionFix. + Findings []dialect.Finding +} + +// NextInput is everything NextAction decides from. It is a struct rather than a +// long parameter list because the decision genuinely needs the whole picture: +// the round, what was observed, what the findings layer extracted, and the +// caller's own local state. +type NextInput struct { + Round state.Round + Obs Observation + Completion CompletionStatus + // Findings is the actionable feedback the crq layer extracted from the same + // observation, already filtered to what can still be acted on. + Findings []dialect.Finding + Global Global + // Primary is the configured primary reviewer's login β€” the one whose review + // spends account quota. It is the only bot an account block can delay, so + // both the rate-limit degrade and the recheck arithmetic need to tell it + // apart from the co-reviewers. + Primary string + // LocalWork reports that the caller holds changes the PR head does not have + // yet (uncommitted, or committed but unpushed). It is what separates "push + // your fixes" from "nothing left to do". + LocalWork bool + // Deferred marks a CodeRabbit rate-limit degrade: the co-reviewers answered + // and CodeRabbit's review is still owed, firing at DeferredUntil. + Deferred bool + DeferredUntil *time.Time + // MinDelay is the floor for Action.At β€” the caller's poll interval. It makes + // a hot loop unrepresentable: every wait is at least this long. + MinDelay time.Duration + // SettleUntil holds back a convergence verdict until the PR has been quiet + // for the configured settle window. Bots deliver in waves β€” a co-reviewer + // auto-reviews a pushed head minutes later, and a primary's detailed + // comments can trail its own completion shell β€” so the first clean + // observation is not yet an answer. Zero disables. + SettleUntil *time.Time +} + +const defaultMinDelay = 15 * time.Second + +func (in NextInput) minDelay() time.Duration { + if in.MinDelay > 0 { + return in.MinDelay + } + return defaultMinDelay +} + +// NextAction reduces the whole review protocol to one instruction. +// +// The order encodes the rules agents get wrong when left to their own devices: +// findings are drained before anything else, the head is held while any +// required reviewer is still pending, and a CodeRabbit rate-limit degrade +// releases the head instead of stalling on it (the queued review fires on the +// new head by itself). Convergence is last, so "done" can only mean every +// required bot answered AND nothing is left to land. +func NextAction(in NextInput, now time.Time) Action { + if !in.Obs.Open { + return Action{Kind: ActionBlocked, Reason: "pr closed"} + } + if in.Obs.Head == "" { + // A transient read failure, not a terminal state β€” come back shortly. + return Action{Kind: ActionWait, Reason: "could not read head", At: in.nextCheck(now, nil, nil)} + } + + pending := pendingBots(in.Completion) + + // 1. Findings first. An agent that starts a new review round on top of + // unresolved feedback burns account quota to be told the same thing. + // + // A threadless finding has no lever the caller can pull β€” nothing clears + // it but a push whose review supersedes it β€” so it can repeat. Suppressing + // it once the tree is dirty was tried and is WORSE: any unrelated dirty + // file then hides a finding the caller never saw at all, which for a + // threadless `review_skipped` is unrecoverable. A repeated instruction is + // visible and annoying; a swallowed finding is silent and harmful, so this + // deliberately errs toward repeating. Clearing them properly needs an + // explicit dismissal, not an inference from local state. + if blocking := BlockingFindings(in.Findings, in.Obs.Head); len(blocking) > 0 { + return Action{ + Kind: ActionFix, + Reason: "actionable findings for this head", + Pending: pending, + Findings: blocking, + } + } + + // 2. A rate-limit degrade releases the head deliberately. The primary's + // review is still owed, but it is queued and will fire against whatever + // head exists when the window opens β€” so holding the current head buys + // nothing and costs a whole window. This is checked BEFORE the generic + // hold below, which would otherwise stall the loop for the full block. + // + // "Released" means released from the PRIMARY only. Every other required + // reviewer still gates the head exactly as it always does, so the degrade + // uses the same DoneExceptWithEvidence rule Feedback applies to + // convergence β€” otherwise a degraded round pushes out from under a Bugbot + // or Macroscope review that is still running. + if in.Deferred { + releasedByDegrade := DoneExceptWithEvidence(in.Completion.ReviewedBy, in.Primary, dialect.CodexBotLogin) + // The co-reviewers' evidence is as fresh here as anywhere else, so the + // same quiet period applies: a review shell can satisfy the degrade while + // its inline findings are still arriving, and moving the head strands + // them on the old commit. + if in.LocalWork && releasedByDegrade && in.settling(now) { + return Action{ + Kind: ActionWait, + Reason: "co-reviewers answered; holding briefly in case trailing findings are still landing", + At: in.nextCheck(now, in.SettleUntil, pending), + Pending: pending, + } + } + switch { + case in.LocalWork && releasedByDegrade: + return Action{ + Kind: ActionPush, + Reason: "co-reviewers answered; primary review deferred and will fire on the new head", + Pending: pending, + } + case in.LocalWork: + return Action{ + Kind: ActionHold, + Reason: "do not push: the primary review is deferred but another required reviewer has not answered for this head", + At: in.nextCheck(now, in.DeferredUntil, pending), + Pending: pending, + } + } + return Action{ + Kind: ActionWait, + Reason: "primary review deferred while the account quota is blocked", + At: in.nextCheck(now, in.DeferredUntil, pending), + Pending: pending, + } + } + + // 3. Required reviewers still pending: the head must not move. Resolving a + // thread does not restart a review; pushing does. + if !in.Completion.Done { + // ...unless the wait already expired. A primary that acknowledges the + // command and then never submits a review leaves the round reviewing + // forever, and `Progress` deliberately does not time it out because the + // legacy Loop owned that deadline. Without an actionable verdict here a + // caller β€” and `crq wait` β€” would idle indefinitely on a bot that + // crashed. Say so instead, and never re-fire: the head was acknowledged. + if in.waitExpired(now) && needsBotReview(in.Completion.ReviewedBy, in.Primary) { + return Action{ + Kind: ActionBlocked, + Reason: "the review was acknowledged for this head but never delivered before the deadline", + Pending: pending, + } + } + kind, reason := ActionWait, "awaiting review" + if in.LocalWork { + // Holding protects a review that is actually happening. With no round + // at all, nothing has been requested for this head, so holding would + // stall the caller AND spend a review window on code it is about to + // replace. Land the work first; the round then covers the real head. + if in.Round.Phase == "" { + return Action{ + Kind: ActionPush, + Reason: "no review has been requested for this head yet; land your work before one is", + } + } + kind, reason = ActionHold, "do not push: a required reviewer has not answered for this head" + } + return Action{Kind: kind, Reason: reason, At: in.nextCheck(now, nil, pending), Pending: pending} + } + + // 4. Everything answered β€” but only once the PR has been quiet long enough + // that a trailing wave would have landed. This gates BOTH outcomes: + // declaring `done` early stops a loop moments before findings arrive, and + // pushing early moves the head out from under a reviewer whose inline + // comments are still landing, stranding them on the old commit. + if in.settling(now) { + return Action{ + Kind: ActionWait, + Reason: "every required reviewer answered; holding briefly in case a trailing review is still landing", + At: in.nextCheck(now, in.SettleUntil, pending), + Pending: pending, + } + } + if in.LocalWork { + return Action{Kind: ActionPush, Reason: "all required reviewers answered on this head"} + } + return Action{Kind: ActionDone, Reason: "converged: no findings and every required reviewer answered"} +} + +// settling reports whether the quiet period after the last evidence is still +// running. +func (in NextInput) settling(now time.Time) bool { + return in.SettleUntil != nil && in.SettleUntil.After(now) +} + +// waitExpired reports whether this round's own wait deadline has passed while it +// is still under review. +// +// Callers must also confirm the PRIMARY is the reviewer still missing: when its +// review has landed and only a co-reviewer is silent, Progress completes the +// round on the primary's word, so reporting a terminal `blocked` here would +// contradict the round crq is about to close. +func (in NextInput) waitExpired(now time.Time) bool { + switch in.Round.Phase { + case state.PhaseFired, state.PhaseReviewing: + return in.Round.WaitDeadline != nil && now.After(*in.Round.WaitDeadline) + } + return false +} + +// nextCheck is when the caller should call again. It is never sooner than +// MinDelay (so a caller cannot hot-loop) and never sooner than a gate that +// definitely prevents progress β€” the account-quota window and this round's own +// retry cooldown, both of which DecideFire enforces anyway. +// +// Two things narrow those gates, and both exist because sleeping through +// feedback is worse than one extra call: +// +// - They only apply to a round still waiting to fire. A fired or reviewing +// round's answer can land at any moment. +// - They only apply when the primary is the ONLY reviewer still owed. Neither +// the account window nor this round's fire cooldown has any hold over a +// co-reviewer, so a round waiting on one must keep the short cadence β€” a +// degraded round otherwise sleeps for the whole block and misses the very +// co-reviewer findings the degrade exists to deliver. +func (in NextInput) nextCheck(now time.Time, extra *time.Time, pending []string) time.Time { + at := now.Add(in.minDelay()).UTC() + if !in.onlyPrimaryPending(pending) || coReviewActive(in.Round) { + return at + } + gate := func(t *time.Time) { + if t != nil && t.After(at) { + at = t.UTC() + } + } + gate(extra) + switch in.Round.Phase { + case state.PhaseQueued, state.PhaseAwaitingRetry: + gate(in.Global.BlockedUntil) + if in.Round.Phase == state.PhaseAwaitingRetry { + gate(in.Round.RetryAt) + } + } + return at +} + +// coReviewActive reports whether a co-reviewer has been commanded for this +// round. +// +// Co-reviewers spend no account quota and take no fire slot, so neither the +// account window nor this round's own fire cooldown gates their answer. A round +// with one in flight must therefore keep the short cadence even when the only +// entry in ReviewedBy is the primary β€” which is the default configuration, and +// is exactly the degrade path: the caller would otherwise sleep until the +// account window opened, hours later, straight through the co-review findings +// the degrade exists to deliver. +func coReviewActive(r state.Round) bool { + for _, co := range r.CoBots { + if co.CommandedAt != nil { + return true + } + } + return false +} + +// onlyPrimaryPending reports whether every reviewer still owed for this head is +// the primary. An empty pending set counts: there is nobody a short cadence +// would catch. +func (in NextInput) onlyPrimaryPending(pending []string) bool { + primary := dialect.NormalizeBotName(in.Primary) + for _, bot := range pending { + if dialect.NormalizeBotName(bot) != primary { + return false + } + } + return true +} + +// pendingBots lists the required bots with no review evidence yet, sorted for a +// stable answer. +func pendingBots(c CompletionStatus) []string { + var out []string + for bot, reviewed := range c.ReviewedBy { + if !reviewed { + out = append(out, bot) + } + } + sort.Strings(out) + return out +} diff --git a/internal/engine/next_test.go b/internal/engine/next_test.go new file mode 100644 index 0000000..e8c561e --- /dev/null +++ b/internal/engine/next_test.go @@ -0,0 +1,398 @@ +package engine + +import ( + "testing" + "time" + + "github.com/kristofferR/coderabbit-queue/internal/dialect" + "github.com/kristofferR/coderabbit-queue/internal/state" +) + +const ( + nextHead = "a21da4aeb" + nextPrimary = "coderabbitai[bot]" + nextCoBot = "bugbot[bot]" +) + +func completionOf(reviewed map[string]bool) CompletionStatus { + done := true + for _, ok := range reviewed { + if !ok { + done = false + } + } + return CompletionStatus{ReviewedBy: reviewed, Done: done} +} + +func openObs() Observation { return Observation{Head: nextHead, Open: true} } + +func finding(commit string) dialect.Finding { + return dialect.Finding{Bot: "coderabbitai[bot]", Title: "nil deref", Commit: commit, ThreadID: "T1"} +} + +// The action table. Each row is a claim about what a caller must do next, and +// together they are the whole contract `crq next` exposes. +func TestNextAction(t *testing.T) { + both := func(cr, codex bool) CompletionStatus { + return completionOf(map[string]bool{nextPrimary: cr, "chatgpt-codex-connector[bot]": codex}) + } + deferredUntil := t0.Add(30 * time.Minute) + + cases := []struct { + name string + in NextInput + want ActionKind + wantAt time.Time // zero = don't check + pending []string + }{ + { + name: "closed pr needs a human", + in: NextInput{Obs: Observation{Head: nextHead, Open: false}}, + want: ActionBlocked, + }, + { + name: "unreadable head is transient, not terminal", + in: NextInput{Obs: Observation{Open: true}, MinDelay: time.Minute}, + want: ActionWait, wantAt: t0.Add(time.Minute), + }, + { + name: "findings come before everything else", + in: NextInput{ + Obs: openObs(), Completion: both(true, true), + Findings: []dialect.Finding{finding(nextHead)}, LocalWork: true, + }, + want: ActionFix, + }, + { + // The rule agents break most: a finding is in hand but a required bot + // has not spoken, so the head must not move. + name: "pending reviewer holds the head when work is staged", + in: NextInput{ + Round: state.Round{Phase: state.PhaseReviewing}, + Obs: openObs(), Completion: both(false, true), LocalWork: true, + MinDelay: time.Minute, + }, + want: ActionHold, wantAt: t0.Add(time.Minute), + pending: []string{"coderabbitai[bot]"}, + }, + { + name: "pending reviewer with nothing staged is just a wait", + in: NextInput{ + Obs: openObs(), Completion: both(false, true), MinDelay: time.Minute, + }, + want: ActionWait, wantAt: t0.Add(time.Minute), + pending: []string{"coderabbitai[bot]"}, + }, + { + // A rate-limit degrade RELEASES the head: the queued CodeRabbit review + // fires against whatever head exists when the window opens, so holding + // costs a whole window and buys nothing. + name: "rate-limit degrade releases the head instead of holding it", + in: NextInput{ + Obs: openObs(), Completion: both(false, true), LocalWork: true, + Deferred: true, DeferredUntil: &deferredUntil, MinDelay: time.Minute, + }, + want: ActionPush, + }, + { + name: "rate-limit degrade with nothing staged waits out the window", + in: NextInput{ + Round: state.Round{Phase: state.PhaseQueued}, + Obs: openObs(), Completion: both(false, true), + Deferred: true, DeferredUntil: &deferredUntil, MinDelay: time.Minute, + }, + want: ActionWait, wantAt: deferredUntil, + pending: []string{"coderabbitai[bot]"}, + }, + { + // The degrade releases the head from the PRIMARY only. Another + // required reviewer is still mid-review, and pushing would restart it. + name: "rate-limit degrade still holds for a pending co-reviewer", + in: NextInput{ + Obs: openObs(), + Completion: completionOf(map[string]bool{ + nextPrimary: false, "chatgpt-codex-connector[bot]": true, nextCoBot: false, + }), + LocalWork: true, + Deferred: true, DeferredUntil: &deferredUntil, MinDelay: time.Minute, + }, + want: ActionHold, wantAt: t0.Add(time.Minute), + pending: []string{nextCoBot, nextPrimary}, + }, + { + // ...and it must keep the short cadence while doing so. The account + // window has no hold over a co-reviewer, so sleeping until it opens + // would miss the very findings the degrade exists to deliver. + name: "rate-limit degrade rechecks soon while a co-reviewer is owed", + in: NextInput{ + Round: state.Round{Phase: state.PhaseQueued}, + Obs: openObs(), + Completion: completionOf(map[string]bool{ + nextPrimary: false, "chatgpt-codex-connector[bot]": true, nextCoBot: false, + }), + Global: Global{BlockedUntil: &deferredUntil}, + Deferred: true, DeferredUntil: &deferredUntil, MinDelay: time.Minute, + }, + want: ActionWait, wantAt: t0.Add(time.Minute), + pending: []string{nextCoBot, nextPrimary}, + }, + { + // A threadless finding is repeated rather than suppressed. Hiding it + // once the tree is dirty was tried and is worse: an unrelated dirty + // file would then bury a finding the caller never saw, and a + // threadless review_skipped has no resolution state to recover from. + name: "threadless finding keeps being reported, not swallowed", + in: NextInput{ + Obs: openObs(), Completion: both(true, true), LocalWork: true, + Findings: []dialect.Finding{{Bot: nextPrimary, Title: "body finding", Commit: nextHead}}, + }, + want: ActionFix, + }, + { + // Holding protects a review that is actually running. With no round, + // nothing was ever requested for this head, so holding would stall the + // caller and spend a window on code it is about to replace. + name: "no round means land the work rather than hold for nobody", + in: NextInput{ + Obs: openObs(), Completion: both(false, false), LocalWork: true, + }, + want: ActionPush, + }, + { + name: "all answered with staged work means push", + in: NextInput{ + Obs: openObs(), Completion: both(true, true), LocalWork: true, + }, + want: ActionPush, + }, + { + name: "all answered with nothing staged is convergence", + in: NextInput{ + Obs: openObs(), Completion: both(true, true), + }, + want: ActionDone, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // Primary is configuration, not scenario: default it so each row + // stays a statement about the review state alone. + if tc.in.Primary == "" { + tc.in.Primary = nextPrimary + } + got := NextAction(tc.in, t0) + if got.Kind != tc.want { + t.Fatalf("NextAction = %q (%s), want %q", got.Kind, got.Reason, tc.want) + } + if !tc.wantAt.IsZero() && !got.At.Equal(tc.wantAt) { + t.Errorf("At = %s, want %s", got.At, tc.wantAt) + } + if len(tc.pending) > 0 { + if len(got.Pending) != len(tc.pending) { + t.Fatalf("Pending = %v, want %v", got.Pending, tc.pending) + } + for i := range tc.pending { + if got.Pending[i] != tc.pending[i] { + t.Fatalf("Pending = %v, want %v", got.Pending, tc.pending) + } + } + } + // A caller must never be told to act in the past. + if got.Kind == ActionWait || got.Kind == ActionHold { + if !got.At.After(t0) { + t.Errorf("At = %s is not in the future", got.At) + } + } + }) + } +} + +// Every wait respects MinDelay, so no caller can hot-loop crq next β€” the floor +// is arithmetic, not a documented request. +func TestNextActionNeverWaitsLessThanMinDelay(t *testing.T) { + passed := t0.Add(-time.Hour) + for _, tc := range []struct { + name string + in NextInput + }{ + {"plain wait", NextInput{Obs: openObs(), Completion: completionOf(map[string]bool{"coderabbitai[bot]": false})}}, + {"elapsed account block", NextInput{ + Obs: openObs(), + Round: state.Round{Phase: state.PhaseQueued}, + Global: Global{BlockedUntil: &passed}, + Completion: completionOf(map[string]bool{"coderabbitai[bot]": false}), + }}, + {"elapsed retry window", NextInput{ + Obs: openObs(), + Round: state.Round{Phase: state.PhaseAwaitingRetry, RetryAt: &passed}, + Completion: completionOf(map[string]bool{"coderabbitai[bot]": false}), + }}, + } { + t.Run(tc.name, func(t *testing.T) { + tc.in.MinDelay = 30 * time.Second + got := NextAction(tc.in, t0) + if want := t0.Add(30 * time.Second); got.At.Before(want) { + t.Errorf("At = %s, want >= %s", got.At, want) + } + }) + } +} + +// A waiting round sleeps past the gates that actually hold it; a round already +// under review does not, because its answer can land at any moment. +func TestNextActionGatesOnlyApplyToWaitingRounds(t *testing.T) { + blocked := t0.Add(40 * time.Minute) + retry := t0.Add(20 * time.Minute) + pending := completionOf(map[string]bool{nextPrimary: false}) + + waiting := NextAction(NextInput{ + Round: state.Round{Phase: state.PhaseAwaitingRetry, RetryAt: &retry}, + Obs: openObs(), + Global: Global{BlockedUntil: &blocked}, + Completion: pending, + Primary: nextPrimary, + MinDelay: time.Minute, + }, t0) + if !waiting.At.Equal(blocked) { + t.Errorf("waiting round: At = %s, want the later gate %s", waiting.At, blocked) + } + + reviewing := NextAction(NextInput{ + Round: state.Round{Phase: state.PhaseReviewing}, + Obs: openObs(), + Global: Global{BlockedUntil: &blocked}, + Completion: pending, + Primary: nextPrimary, + MinDelay: time.Minute, + }, t0) + if !reviewing.At.Equal(t0.Add(time.Minute)) { + t.Errorf("reviewing round: At = %s, want a poll-interval recheck, not the account block", reviewing.At) + } +} + +// Those same gates belong to the primary alone. A queued round that is also +// waiting on a co-reviewer must keep the short cadence: neither the account +// window nor this round's fire cooldown has any hold over that bot, so sleeping +// until they clear would sleep through its answer. +func TestNextActionKeepsShortCadenceForPendingCoReviewers(t *testing.T) { + blocked := t0.Add(40 * time.Minute) + retry := t0.Add(20 * time.Minute) + + got := NextAction(NextInput{ + Round: state.Round{Phase: state.PhaseAwaitingRetry, RetryAt: &retry}, + Obs: openObs(), + Global: Global{BlockedUntil: &blocked}, + Completion: completionOf(map[string]bool{nextPrimary: false, nextCoBot: false}), + Primary: nextPrimary, + MinDelay: time.Minute, + }, t0) + if got.Kind != ActionWait { + t.Fatalf("NextAction = %q (%s), want %q", got.Kind, got.Reason, ActionWait) + } + if !got.At.Equal(t0.Add(time.Minute)) { + t.Errorf("At = %s, want a poll-interval recheck while %s is still owed", got.At, nextCoBot) + } +} + +// Findings carried from an older commit that have no thread to resolve must not +// pin the loop on "fix" forever β€” BlockingFindings already drops them, and +// NextAction must move on to the review state. +func TestNextActionIgnoresUnresolvableStaleFindings(t *testing.T) { + stale := dialect.Finding{Bot: "coderabbitai[bot]", Title: "old", Commit: "deadbeef1"} + got := NextAction(NextInput{ + Obs: openObs(), + Completion: completionOf(map[string]bool{"coderabbitai[bot]": true}), + Findings: []dialect.Finding{stale}, + }, t0) + if got.Kind != ActionDone { + t.Fatalf("NextAction = %q (%s), want %q", got.Kind, got.Reason, ActionDone) + } +} + +// The account window gates the PRIMARY's review and nothing else. In the default +// configuration only the primary is required, so a co-reviewer commanded for +// this round never appears in ReviewedBy β€” and the caller would be told to sleep +// until the window opened, hours later, straight through the co-review findings +// the degrade exists to deliver. +func TestNextActionKeepsShortCadenceForACommandedCoReviewer(t *testing.T) { + blocked := t0.Add(90 * time.Minute) + commanded := t0.Add(-time.Minute) + round := state.Round{Phase: state.PhaseQueued} + round.CoBots = map[string]state.CoBotRound{ + "chatgpt-codex-connector": {CommandID: 42, CommandedAt: &commanded}, + } + + got := NextAction(NextInput{ + Round: round, + Obs: openObs(), + Global: Global{BlockedUntil: &blocked}, + Completion: completionOf(map[string]bool{nextPrimary: false}), + Primary: nextPrimary, + MinDelay: time.Minute, + }, t0) + if !got.At.Equal(t0.Add(time.Minute)) { + t.Errorf("At = %s, want a poll-interval recheck while a co-review is in flight", got.At) + } +} + +// A primary that acknowledges the command and then never submits a review leaves +// the round reviewing forever β€” Progress deliberately does not time that out, +// because the legacy Loop owned the deadline. Without an actionable verdict here +// the newly recommended flow idles indefinitely on a bot that crashed. +func TestNextActionReportsAnExpiredReviewWait(t *testing.T) { + expired := t0.Add(-time.Minute) + got := NextAction(NextInput{ + Round: state.Round{Phase: state.PhaseReviewing, WaitDeadline: &expired}, + Obs: openObs(), + Completion: completionOf(map[string]bool{nextPrimary: false}), + Primary: nextPrimary, + MinDelay: time.Minute, + }, t0) + if got.Kind != ActionBlocked { + t.Fatalf("NextAction = %q (%s), want %q", got.Kind, got.Reason, ActionBlocked) + } + + // An unexpired deadline is still just a wait. + live := t0.Add(time.Hour) + if got := NextAction(NextInput{ + Round: state.Round{Phase: state.PhaseReviewing, WaitDeadline: &live}, + Obs: openObs(), + Completion: completionOf(map[string]bool{nextPrimary: false}), + Primary: nextPrimary, + MinDelay: time.Minute, + }, t0); got.Kind != ActionWait { + t.Fatalf("NextAction = %q (%s), want %q while the deadline is live", got.Kind, got.Reason, ActionWait) + } +} + +// Bots deliver in waves, so the first clean observation is not an answer yet. +// The legacy loop held its verdict for a settle window; a stateless caller needs +// the same guarantee or it stops moments before the findings land. +func TestNextActionHoldsConvergenceThroughTheSettleWindow(t *testing.T) { + settleUntil := t0.Add(90 * time.Second) + got := NextAction(NextInput{ + Obs: openObs(), + Completion: completionOf(map[string]bool{nextPrimary: true}), + Primary: nextPrimary, + SettleUntil: &settleUntil, + MinDelay: time.Minute, + }, t0) + if got.Kind != ActionWait { + t.Fatalf("NextAction = %q (%s), want %q inside the settle window", got.Kind, got.Reason, ActionWait) + } + if !got.At.Equal(settleUntil) { + t.Errorf("At = %s, want the settle boundary %s", got.At, settleUntil) + } + + // Once quiet, it converges. + passed := t0.Add(-time.Second) + if got := NextAction(NextInput{ + Obs: openObs(), + Completion: completionOf(map[string]bool{nextPrimary: true}), + Primary: nextPrimary, + SettleUntil: &passed, + }, t0); got.Kind != ActionDone { + t.Fatalf("NextAction = %q (%s), want %q once settled", got.Kind, got.Reason, ActionDone) + } +} diff --git a/internal/gh/github.go b/internal/gh/github.go index ed7f290..0b81662 100644 --- a/internal/gh/github.go +++ b/internal/gh/github.go @@ -792,6 +792,11 @@ type Pull struct { Head struct { SHA string `json:"sha"` Ref string `json:"ref"` + // Repo is the head's repository, which differs from the base on a fork + // PR. A contributor's checkout has a remote for THIS, not the base. + Repo struct { + FullName string `json:"full_name"` + } `json:"repo"` } `json:"head"` Merged bool `json:"merged"` } diff --git a/internal/gh/testclient.go b/internal/gh/testclient.go new file mode 100644 index 0000000..091fc88 --- /dev/null +++ b/internal/gh/testclient.go @@ -0,0 +1,28 @@ +package gh + +import "net/http" + +// NewTestClient builds a client aimed at a local test server. +// +// It exists so packages layered above gh can assert their own request +// behaviour β€” which calls they make, and how many β€” without reaching the +// network. That matters because the account shares one REST budget across the +// daemon and every agent, so "does this code path write?" is a correctness +// property worth a test, not just a performance note. +// +// Retries and backoff are wound down to keep tests fast; everything else, +// including the ETag/conditional-GET cache, behaves exactly as in production. +func NewTestClient(baseURL string, client *http.Client) *GitHub { + if client == nil { + client = http.DefaultClient + } + return &GitHub{ + token: "test-token", + httpClient: client, + apiBase: baseURL, + graphBase: baseURL + "/graphql", + maxRetries: 1, + maxWait: 0, + backoffBase: 0, + } +} diff --git a/internal/state/dashboard_sync_test.go b/internal/state/dashboard_sync_test.go new file mode 100644 index 0000000..831e596 --- /dev/null +++ b/internal/state/dashboard_sync_test.go @@ -0,0 +1,169 @@ +package state + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strconv" + "strings" + "sync" + "testing" + + gh "github.com/kristofferR/coderabbit-queue/internal/gh" +) + +// issueServer is a minimal gate-issue endpoint that records how it was used. +// The counts ARE the assertions: PATCHes are charged writes against a shared +// account budget and are the endpoint class that trips GitHub's secondary +// limits, so "how many writes did this cost" is the property under test. +type issueServer struct { + mu sync.Mutex + patches int + gets int + notModified int + title string + body string + etag string +} + +func (s *issueServer) start(t *testing.T) (*httptest.Server, *gh.GitHub) { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !strings.Contains(r.URL.Path, "/issues/") { + http.NotFound(w, r) + return + } + s.mu.Lock() + defer s.mu.Unlock() + switch r.Method { + case http.MethodPatch: + var payload struct { + Title string `json:"title"` + Body string `json:"body"` + } + if err := json.NewDecoder(r.Body).Decode(&payload); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + s.patches++ + s.title, s.body = payload.Title, payload.Body + s.etag = "" + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(map[string]any{"number": 1, "title": s.title, "body": s.body}) + case http.MethodGet: + s.gets++ + if s.etag == "" { + s.etag = `"v` + strconv.Itoa(s.patches) + `"` + } + w.Header().Set("ETag", s.etag) + // Model the real endpoint: an unchanged issue answers 304 and costs + // no quota, which is what makes reading-before-writing affordable. + if r.Header.Get("If-None-Match") == s.etag { + s.notModified++ + w.WriteHeader(http.StatusNotModified) + return + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(map[string]any{"number": 1, "title": s.title, "body": s.body, "state": "open"}) + default: + http.Error(w, "unexpected method", http.StatusMethodNotAllowed) + } + })) + t.Cleanup(srv.Close) + return srv, gh.NewTestClient(srv.URL, srv.Client()) +} + +func syncTestState(t *testing.T) State { + t.Helper() + st := New() + st.Account.Scope = "owner" + if _, err := st.NewRound("owner/repo", 7, "abcdef123", t0); err != nil { + t.Fatal(err) + } + st.Normalize(t0) + return st +} + +// Every crq command that touches state syncs the dashboard, including +// read-mostly paths like Enqueue that usually change nothing. An unconditional +// PATCH there made a plain `crq next` a write, so an unchanged sync must cost +// nothing. +func TestSyncDashboardWritesOnlyOnChange(t *testing.T) { + srv := &issueServer{} + _, client := srv.start(t) + cfg := StoreConfig{GateRepo: "owner/state", StateRef: "crq-state-v3", DashboardIssue: 1, Scope: []string{"owner"}} + store := NewGitStateStore(cfg, client, nil) + ctx := context.Background() + + st := syncTestState(t) + for i := 0; i < 4; i++ { + if err := store.SyncDashboard(ctx, st); err != nil { + t.Fatalf("sync %d: %v", i, err) + } + } + srv.mu.Lock() + patches := srv.patches + srv.mu.Unlock() + if patches != 1 { + t.Fatalf("four identical syncs wrote %d times, want exactly 1", patches) + } + srv.mu.Lock() + notModified := srv.notModified + srv.mu.Unlock() + if notModified == 0 { + t.Error("the repeat syncs must revalidate conditionally, so they cost no quota") + } + + // Real movement must still reach the issue. + if _, err := st.NewRound("owner/repo", 8, "beefcafe1", t0); err != nil { + t.Fatal(err) + } + st.Normalize(t0) + if err := store.SyncDashboard(ctx, st); err != nil { + t.Fatal(err) + } + srv.mu.Lock() + patches = srv.patches + srv.mu.Unlock() + if patches != 2 { + t.Fatalf("a changed dashboard wrote %d times total, want 2", patches) + } +} + +// A short-lived `crq next` starts cold every time, so the check has to work +// across processes: if the daemon already wrote this exact content, there is +// nothing left to write. +func TestSyncDashboardSkipsWriteWhenTheIssueAlreadyMatches(t *testing.T) { + srv := &issueServer{} + httpSrv, client := srv.start(t) + cfg := StoreConfig{GateRepo: "owner/state", StateRef: "crq-state-v3", DashboardIssue: 1, Scope: []string{"owner"}} + ctx := context.Background() + st := syncTestState(t) + + // One process (think: the daemon) publishes the dashboard. + if err := NewGitStateStore(cfg, client, nil).SyncDashboard(ctx, st); err != nil { + t.Fatal(err) + } + srv.mu.Lock() + after := srv.patches + srv.mu.Unlock() + if after != 1 { + t.Fatalf("initial publish wrote %d times, want 1", after) + } + + // A genuinely cold second process against the same issue: its own client, so + // it shares no ETag cache with the first. + cold := NewGitStateStore(cfg, gh.NewTestClient(httpSrv.URL, httpSrv.Client()), nil) + if err := cold.SyncDashboard(ctx, st); err != nil { + t.Fatal(err) + } + srv.mu.Lock() + defer srv.mu.Unlock() + if srv.patches != 1 { + t.Fatalf("a cold process re-wrote an already-current dashboard (%d writes), want 1", srv.patches) + } + if srv.gets == 0 { + t.Fatal("the cross-process check must actually read the issue") + } +} diff --git a/internal/state/store.go b/internal/state/store.go index ffc3c63..e67e557 100644 --- a/internal/state/store.go +++ b/internal/state/store.go @@ -86,6 +86,10 @@ type GitStateStore struct { cfg StoreConfig gh *gh.GitHub log Logger + + // syncMu serializes the read-then-write in SyncDashboard, so concurrent + // syncs cannot both see a stale gate issue and both write it. + syncMu sync.Mutex } func NewGitStateStore(cfg StoreConfig, client *gh.GitHub, log Logger) *GitStateStore { @@ -237,6 +241,24 @@ func (s *GitStateStore) compareAndSwap(ctx context.Context, st *State, rev Revis return err } +// SyncDashboard renders the gate issue and writes it only when the issue does +// not already say exactly that. +// +// Every caller that touches state calls this, including read-mostly paths like +// Enqueue that usually report ErrNoChange β€” so an unconditional PATCH here made +// a plain `crq next` a write, and writes are the endpoint class that trips +// GitHub's *secondary* rate limits (the reason enqueues were batched in the +// first place). +// +// The check reads the issue rather than remembering what this process last +// wrote. A memo would be cheaper still, but it would also stop the dashboard +// ever self-healing: an issue edited by hand, or by a binary running different +// code, would stay wrong until state happened to change. Reading costs nothing +// to be right β€” GetIssue is a conditional GET, so an unchanged issue answers +// 304 from cache and spends no quota. +// +// A read failure is never fatal: fall through and PATCH, which is the old +// behavior. func (s *GitStateStore) SyncDashboard(ctx context.Context, st State) error { if err := s.cfg.requireDashboard(); err != nil { return err @@ -245,7 +267,18 @@ func (s *GitStateStore) SyncDashboard(ctx context.Context, st State) error { if err != nil { return err } - return s.gh.PatchIssue(ctx, s.cfg.GateRepo, s.cfg.DashboardIssue, RenderTitle(st), body) + title := RenderTitle(st) + + // Held across read-then-write so two concurrent syncs cannot both observe a + // stale issue and both write it. + s.syncMu.Lock() + defer s.syncMu.Unlock() + + if issue, err := s.gh.GetIssue(ctx, s.cfg.GateRepo, s.cfg.DashboardIssue); err == nil && + issue.Body == body && issue.Title == title { + return nil + } + return s.gh.PatchIssue(ctx, s.cfg.GateRepo, s.cfg.DashboardIssue, title, body) } // MemoryStore is the in-memory store used by tests and the fake-GitHub harness. diff --git a/llms.txt b/llms.txt index c12923d..922b884 100644 --- a/llms.txt +++ b/llms.txt @@ -2,7 +2,8 @@ crq is the repo's CLI for autonomous CodeRabbit/Codex pull-request review loops. -Core rule: never post `@coderabbitai review` directly. Use `crq loop`. +Core rule: never post `@coderabbitai review` directly. To drive a PR, call `crq next` and do exactly +what its `.action` says (see Agent Loop below); everything else is reference material. Do not bypass crq to read review status, either. Never hand-poll the GitHub API (`gh api .../pulls/N/reviews|comments`, looping on the head) to wait for a review @@ -37,33 +38,80 @@ CodeRabbit native auto-review must be off. ## Agent Loop -Drain-first invariant: before waiting for a new review, run `crq feedback` and address every -current finding. Fix genuine issues, validate locally, and immediately resolve each addressed thread -(or record and resolve a decline). Commit and push only when every configured required reviewer has a -`true` `.reviewed_by` value for the current head; otherwise hold the head. Repeat until `.findings` is -empty, then start `crq loop`. After any loop return, inspect -`.findings` before the exit code; non-empty findings are immediate work even if a required bot -timed out. Never describe the PR as β€œwaiting for review” while actionable findings are present. -The loop also returns as soon as any feedback bot reports a finding; it does not wait for the -remaining required bots or a shared review slot. Fix those findings locally, but HOLD THE HEAD: -resolve each addressed thread immediately, but do not commit or push while any `.reviewed_by` value -is false, because changing the head restarts the pending checks while thread resolution does not. -Exception: when `.coderabbit_deferred` is true and every non-CodeRabbit required reviewer is true, -the round is Codex-only (CodeRabbit rate-limited) β€” fix, push, and loop again without holding the -head; the queued CodeRabbit review is untouched. -Keep the queued review alive and poll `crq feedback` with the same `CRQ_REQUIRED_BOTS`. When every -required bot is true, fix and resolve the rest, then commit all fixes once and push once. - -`crq loop` also enforces this invariant for new rounds: unresolved findings are returned before -the PR is queued or a review slot is consumed. -Thread-less review-body findings from a previous commit do not block after fixes are pushed; the -next current-head review supersedes them or reports the finding again. +**Call `crq next`, do exactly what `.action` says, call it again.** That is the entire loop. Every +judgement an agent used to make β€” how long to wait, whether the head may move, whether the round is +finished β€” is a value crq computes. + +```bash +crq next OWNER/REPO PR_NUMBER +``` + +| `.action` | what to do | +|---|---| +| `fix` | Fix `.findings[]`, validate locally, then `crq resolve` each addressed `.thread_id` (or `crq decline` with a reason). Call again. | +| `hold` | Do NOT commit or push: a required reviewer has not answered for this head, and moving the head restarts its review (resolving threads does not). Call again at `.recheck_after`. | +| `push` | The head is released. Commit and push the accumulated fixes once. Call again. | +| `wait` | Nothing to do until `.recheck_after`. | +| `done` | Converged. | +| `blocked` | Needs a human; `.reason` says why (e.g. the PR was closed). | + +`crq next` always exits 0 on success β€” read `.action`, never the exit code. It is non-blocking and +idempotent, and it advances the queue by one step as a side effect, so a PR in a repo outside the +autoreview fleet still progresses. There is no long-lived process to keep alive: if the harness kills +the agent mid-loop, the next `crq next` returns the correct action from persisted state. + +## Waiting + +On `wait` or `hold` there is nothing to do yet. Do not sleep, poll, or guess a delay β€” hand the +wait over and end your turn: + +```bash +crq wait OWNER/REPO PR_NUMBER +``` + +It blocks until there IS something to do (`fix`, `push`, `done`, `blocked`), then prints that same +JSON and exits 0. Run it as your harness's background task: its **exit is the wake event**, so you +burn no tokens idling. + +It owns nothing β€” no round, no state, no writes β€” so being killed costs only the process. If that +happens, run it again or call `crq next`; the answer comes from persisted state either way. While +idle it watches the shared state ref with a conditional request that costs no rate-limit quota. + +`crq next --wait` is the same thing inline, for a human at a terminal. + +Two rules this replaces, which agents previously had to remember: + +- **Never invent a delay.** `.recheck_after` is computed from the account-quota window, the round's + retry cooldown and the poll interval, and is always at least one poll interval away. Do not + substitute a timer, guessed review latency, or a `crq status` polling loop. +- **Never decide to push yourself.** `hold` vs `push` is crq's answer. It already accounts for the + rate-limit degrade (a Codex-only round while CodeRabbit is blocked releases the head, because the + queued CodeRabbit review fires against whatever head exists when the window opens). + +`.local_work` is what separates `push` from `done`: crq checks whether the working copy holds changes +the PR head lacks. Run `crq next` inside the repository checkout so that answer is accurate; +`.local_work_reason` says when it could not be determined. + +`crq next --wait` blocks through the states you cannot act on and returns the first actionable +instruction. It shares one code path with the non-blocking form. + +### crq loop (interactive/one-shot) + +`crq loop` is the older primitive: it triggers a round, blocks until feedback lands, and returns one +report. It remains supported for humans and one-shot scripts, but an agent driving a PR should use +`crq next` β€” `loop` requires the caller to interpret exit codes and to enforce the hold-the-head and +drain-first rules by hand. ```bash crq loop OWNER/REPO PR_NUMBER > crq-feedback.json rc=$? ``` +`crq loop` enforces the drain-first invariant for new rounds: unresolved findings are returned before +the PR is queued or a review slot is consumed. Thread-less review-body findings from a previous +commit do not block after fixes are pushed; the next current-head review supersedes them or reports +the finding again. + `crq loop` queues the PR and triggers CodeRabbit when the account can spend a review. Alongside it crq runs its **co-reviewers** β€” Codex, Cursor Bugbot and Macroscope (`CRQ_COBOTS`, all enabled by default). None of them spends CodeRabbit quota or takes the fire slot. Each has a trigger mode @@ -91,7 +139,7 @@ CodeRabbit is rate-limited. Only after every `CRQ_REQUIRED_BOTS` entry is true for the current head should you fix and resolve the rest, then commit and push the combined fixes once. Convergence is reported only after that same condition is met. -Exit codes: +Exit codes (`crq loop` only β€” `crq next` always exits 0 and answers in `.action`): - `0`: done; converged or no actionable findings. Check `.coderabbit_deferred`: when true (with `.status == "deferred"`), Codex reviewed clean but the CodeRabbit review is still owed β€” this is progress, NOT convergence (`.converged` stays false). Re-run `crq loop` after @@ -135,14 +183,15 @@ Resolve addressed threads on GitHub (crq keys off GitHub resolution state; a fin reappearing until its thread is resolved there): ```bash -crq resolve OWNER/REPO PR_NUMBER --thread THREAD_ID +crq resolve THREAD_ID [THREAD_ID...] ``` -Decline a finding you are not addressing: post the reason on its thread (left unresolved; add -`--resolve` to also close it) instead of leaving it silently open: +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: ```bash -crq decline OWNER/REPO PR_NUMBER --thread THREAD_ID --reason "why declined" +crq decline THREAD_ID [THREAD_ID...] --reason "why declined" ``` If the bot replies contesting a decline you resolved ("I'm retaining the finding: ..."), crq @@ -166,19 +215,15 @@ re-emitted. Unresolved cross-commit threads remain visible. Long waits are not hangs. During queue/rate-limit waits, missing required-bot reviews, and network/GitHub outages, crq logs progress to stderr; stdout stays reserved for the final JSON. -Keep the loop alive: harnesses often kill plain background shell jobs between agent turns, which -orphans the wait. Run `crq loop` under the harness's persistent long-running-task primitive (in -Claude Code: the Monitor tool with `persistent: true`, e.g. -`set +e; crq loop OWNER/REPO PR > crq-feedback.json; echo "CRQ_EXIT:$?"` so the exit line is the -completion event β€” the `set +e` keeps an errexit shell from dying on the meaningful non-zero exits -(10 = findings, 2 = timeout) before it can echo). If a runner is killed anyway, the PR stays -enqueued β€” re-running the same `crq loop` is -idempotent and re-attaches to the wait. Never replace the runner with a `crq status` polling loop. +Keeping a loop alive is not your problem. `crq next` returns immediately, and `crq wait` owns +nothing, so neither leaves anything for a harness to orphan: if either is killed, run it again and +the answer comes from persisted state. Never substitute a `crq status` polling loop, a `gh api` +poll, a timer, or a guessed review latency for `crq wait`. Agent communication rule: do not relay every stderr progress line or send repeated "still waiting" -updates. Tell the user once when a long `crq loop` wait begins, then stay quiet until the state -changes (fired, feedback wait, findings, convergence, timeout, rate-limit/window change, network -outage/recovery), the user asks for status, or at least 10 minutes have passed with no update. +updates. Report a real state change β€” fired, findings, `push`, convergence, timeout, a +rate-limit/window change, a network outage or recovery β€” or answer when the user asks. If the only +new information is elapsed time on the same wait, stay quiet. Fleet review: diff --git a/skills/coderabbit-queue/SKILL.md b/skills/coderabbit-queue/SKILL.md index 0b97cbd..cab6ef8 100644 --- a/skills/coderabbit-queue/SKILL.md +++ b/skills/coderabbit-queue/SKILL.md @@ -14,171 +14,108 @@ directly will stampede the same quota. `crq` owns that mechanical loop: 4. emit normalized JSON findings or report convergence, 5. resolve the review threads the agent says it addressed. -## The Rule +## The Loop -Never post `@coderabbitai review` directly. Use `crq loop` for an agent round: +**Call `crq next`, do exactly what `.action` says, call it again.** That is the whole agent loop. +Do not design one of your own. ```bash -crq loop "$REPO" "$PR" > crq-feedback.json +crq next "$REPO" "$PR" ``` -Don't bypass crq to read review status either: never hand-poll the GitHub API -(`gh api .../pulls/N/reviews|comments`, looping on the head) to wait for a review -or its outcome. That drains the shared account-wide GitHub REST quota β€” also spent -by the `crq autoreview` daemon and every other agent, so it exhausts fast β€” and -competes with crq's own polling. Use `crq loop` (waits and returns findings), -`crq feedback` (current findings, no trigger), or `crq status` (queue/quota). +| `.action` | what to do | +|---|---| +| `fix` | Fix `.findings[]`, validate locally, then `crq resolve` each addressed `.thread_id` (or `crq decline` with a reason). Call again. | +| `hold` | Do NOT commit or push β€” a required reviewer has not answered for this head, and moving the head restarts its review (resolving threads does not). Call again at `.recheck_after`. | +| `push` | The head is released. Commit and push the accumulated fixes once. Call again. | +| `wait` | Nothing to do until `.recheck_after`. | +| `done` | Converged. Report and stop. | +| `blocked` | Needs a human; `.reason` says why (e.g. the PR was closed). | -Before starting, check local readiness: +`crq next` always exits 0 on success: read `.action`, never the exit code. It is **non-blocking and +idempotent**, and it advances the queue by one step as a side effect β€” so a PR in a repo outside the +autoreview fleet still progresses, and running it alongside the daemon is safe. -```bash -crq doctor -``` +Three things this deliberately takes away from you: + +- **Choosing a delay.** `.recheck_after` is computed by crq from the account-quota window, the + round's retry cooldown and the poll interval, and is never less than one poll interval away. Never + invent one: hand the wait to `crq wait` (below), and only if your harness cannot run a background + task, schedule a single wake at exactly `.recheck_after`. Never poll in-chat and never loop on + `crq status` or `gh api`. +- **Deciding when to push.** `hold` vs `push` is crq's answer. It already accounts for the + rate-limit degrade: a Codex-only round while CodeRabbit is blocked returns `push`, because the + queued CodeRabbit review fires against whatever head exists when the window opens. +- **Keeping a process alive.** There is none. If the harness kills you mid-loop, the next `crq next` + returns the correct action from persisted state. Nothing to re-attach, nothing to babysit. -`crq doctor` emits JSON covering crq config, `gh`, optional CodeRabbit CLI availability, -and `CODERABBIT_API_KEY` presence for headless local review. - -Exit codes: - -- `0`: converged or no actionable findings. Check `.coderabbit_deferred` first: when true (with - `.status == "deferred"`), Codex reviewed clean while CodeRabbit is rate-limited β€” the CodeRabbit - review is still owed and `.converged` is false. Treat it as progress, not convergence: re-run - `crq loop` after `.coderabbit_deferred_until` or on the next push. -- `10`: actionable findings were written to JSON. When `.coderabbit_deferred` is true and all - non-CodeRabbit required reviewers are true, these are Codex-only findings during a CodeRabbit - rate-limit window: fix, push, and loop again immediately instead of holding the head β€” the queued - CodeRabbit review fires by itself once the window opens. -- `2`: timed out waiting for feedback - -Rate-limit degrade (default on, `CRQ_RL_CODEX_DEGRADE=0` disables): when CodeRabbit is -rate-limited and Codex demonstrably reviews the PR, the loop returns Codex feedback promptly -instead of waiting out the window, and the pump posts the Codex command for blocked rounds while -keeping the CodeRabbit review queued. - -## Drain Findings Before Waiting - -An autonomous review loop is a work loop, not a review-status waiter. Before starting or -restarting a review round, drain all currently actionable feedback: - -1. run `crq feedback "$REPO" "$PR"`, -2. if `.findings` is non-empty, verify and fix genuine findings immediately, -3. validate locally, then immediately resolve each addressed thread (or record and resolve a decline), -4. if any required reviewer is still pending on the current head, do not commit or push, -5. after every required reviewer finishes, fix and resolve the remaining findings, then commit and - push all accumulated fixes once, -6. repeat until current feedback is empty, -7. only then call `crq loop` for a fresh review round. - -`crq loop` enforces this for a new round by returning existing findings before it queues or -waits. After any loop result, inspect `.findings` **before** interpreting the exit code. Findings -always mean work nowβ€”even if a required reviewer timed out. Never report β€œstill waiting” while -the JSON already contains actionable findings. -The loop returns as soon as any configured feedback bot reports a finding, even if another -required bot is pending. Fix and validate it locally immediately, but **hold the PR head** while -any `.reviewed_by` value is false: resolve the addressed thread immediately, but do not commit or -push, because changing the head restarts the pending checks while resolving a thread does not. Keep -the queued review alive and poll `crq feedback` using the same `CRQ_REQUIRED_BOTS`. Once every required -bot is true, fix and resolve the remaining findings, combine all fixes into one commit, and push once. - -Thread-less review-body summaries from a previous commit have no GitHub thread to resolve. After -their fixes are pushed they do not gate the next round; the current-head review supersedes them -or re-reports anything that remains valid. - -The agent fixes genuine findings, validates, resolves addressed threads immediately, waits for every -required reviewer, fixes and resolves the rest, then commits and pushes once before calling `crq loop` -again. A round counts only after its findings are drained and the resulting head has received the -required reviews. - -Minimal implementation: +`.local_work` separates `push` from `done`: crq checks whether the working copy holds changes the PR +head lacks. **Run `crq next` from inside the repository checkout** so that answer is accurate; +`.local_work_reason` says when it could not be determined. + +## Waiting + +On `wait` or `hold`, do not sleep, poll, or guess a delay. Hand the wait over and end your turn: ```bash -set +e -crq loop "$REPO" "$PR" > crq-feedback.json -rc=$? -set -e - -case "$rc" in - 0) echo "converged" ;; - 10) jq '.findings[] | {bot,severity,path,line,title,thread_id,source}' crq-feedback.json ;; - 2) - if jq -e '.findings | length > 0' crq-feedback.json >/dev/null; then - jq '.findings[] | {bot,severity,path,line,title,thread_id,source}' crq-feedback.json - else - echo "timed out with no findings; retry later" - fi - ;; - *) exit "$rc" ;; -esac +crq wait "$REPO" "$PR" ``` -Long waits are expected when the queue is blocked, GitHub is rate-limited, a required bot has not -reviewed yet, or the network is down. crq logs progress to stderr; do not kill it just because stdout -is quiet. - -If an extraction-only bot such as Codex reports a finding first, `crq loop` emits that finding early -so work can begin, but it does not mark the round complete and leaves the queued review alive. Fix and -validate locally, resolve its thread immediately, then hold the head until every `CRQ_REQUIRED_BOTS` -reviewer has reviewed it. An early `crq feedback` snapshot is actionable work, not permission to commit -or push. - -Codex's clean summary (`Codex Review: Didn't find any major issues. Keep them coming!`) is a successful -review signal, not a finding. crq suppresses it when Codex is extraction-only and counts it in -`reviewed_by` when `chatgpt-codex-connector[bot]` is included in `CRQ_REQUIRED_BOTS`. The signal is -accepted only when it was posted after the persisted wait for the current head began, because GitHub -issue comments do not contain a commit SHA. - -## Keeping the Loop Alive in an Agent Harness - -A single `crq loop` call can wait an hour or more when the queue is deep or the account is -rate-limited. Agent harnesses commonly kill plain background shell jobs between turns, which -silently orphans the wait. Run `crq loop` under the harness's *persistent* long-running-task -primitive instead of a fire-and-forget background shell. In Claude Code that is the Monitor tool -with `persistent: true`, redirecting the findings JSON to a file and emitting one final event line: - -```js -Monitor({ - command: 'set +e; crq loop OWNER/REPO PR > /path/to/crq-feedback.json; echo "CRQ_EXIT:$?"', - description: 'crq review loop on OWNER/REPO#PR', - persistent: true, -}) -``` +It blocks until there IS something to do (`fix`, `push`, `done`, `blocked`), prints that same JSON +and exits 0. Run it as your harness's background task β€” its **exit is the wake event**, so you burn +no tokens idling and never narrate a countdown. + +It owns no round and holds no state, so being killed costs only the process β€” just run it again (or +call `crq next`). While idle it watches the shared state ref with a conditional request that spends +no rate-limit quota. It is read-only in the steady state, but if nothing is advancing your PR (no +round for the head, or no daemon holding the leader lease) it drives the queue itself rather than +wait for nobody β€” which can request a review. -The `set +e` matters: `crq loop` reports actionable outcomes as non-zero exits (10 = findings, -2 = timeout), and a shell with errexit inherited would exit before the `echo` β€” the completion -event would never arrive even though `crq-feedback.json` was written. +`crq next --wait` is the same wait inline, for a human at a terminal. All three share one decision +function, so they cannot disagree. -The `CRQ_EXIT:` line is the completion event (map it to the exit codes above); crq's stderr -progress stays in the task's output file for diagnosis without generating event noise. +## Never Bypass crq -Do **not** replace this wait with a timer, scheduled wake-up, reminder, heartbeat automation, or a -guessed CodeRabbit response delay. Codex and CodeRabbit have independent latency and quota windows, -so time-based wake-ups routinely run before the required review exists. The persistent `crq loop` -process is the waiter. In a harness without a dedicated monitor primitive, keep the foreground PTY -session attached; if the turn is interrupted, re-run the same idempotent `crq loop` command to resume. +Never post `@coderabbitai review` directly β€” crq is the only trigger, because CodeRabbit's review +limit is account-wide and direct posts stampede it. -If a loop runner is killed anyway, nothing is lost: the PR stays enqueued. Re-running the same -`crq loop` command is safe and re-attaches to the wait β€” enqueueing is idempotent. Do not -substitute a hand-rolled `crq status` polling loop for the runner. +Never hand-poll the GitHub API (`gh api .../pulls/N/reviews|comments`, looping on the head) to wait +for a review or learn its outcome. That drains the shared account-wide GitHub REST quota β€” also spent +by the `crq autoreview` daemon and every other agent, so it exhausts fast β€” and competes with crq's +own polling. Use `crq next` (the loop), `crq wait` (block until actionable), `crq feedback` +(current findings, no trigger), or `crq status` (queue/quota). -## User-Facing Updates During Waits +Before starting, check local readiness: + +```bash +crq doctor +``` -This section overrides generic agent progress-update habits. While `crq loop` is in an ordinary -waiting state, do not send periodic heartbeat updates to the user and do not narrate repeated stderr -lines such as "waiting for a review slot" or "waiting for review feedback". +`crq doctor` emits JSON covering crq config, `gh`, optional CodeRabbit CLI availability, and +`CODERABBIT_API_KEY` presence for headless local review. -Send a user update only for a real state change or action: +## crq loop (interactive/one-shot) -- review command fired -- feedback wait started or resumed for a head -- findings, convergence, timeout, or unexpected failure returned -- rate-limit/window state is first discovered or changes materially -- network outage or recovery is detected -- findings were fixed, declined, or resolved -- the user asks for status +`crq loop` is the older primitive: it triggers a round, blocks until feedback lands, and returns one +report with a frozen exit code (0 converged/skipped, 10 findings, 2 timeout). It remains supported +for humans and one-shot scripts. -If the only new information is elapsed time on the same wait, stay silent. If crq reports a long -blocked-until window, summarize it once with the absolute unblock time, then stay silent until the -state changes or the user asks. +An agent driving a PR should use `crq next` instead. `crq loop` requires the caller to interpret exit +codes, enforce the drain-first and hold-the-head rules by hand, and keep a long-lived process alive +across turns β€” the three things that go wrong. Use `crq next` plus `crq wait` instead. + +Rate-limit degrade (default on, `CRQ_RL_CODEX_DEGRADE=0` disables): when CodeRabbit is rate-limited +and Codex demonstrably reviews the PR, crq returns Codex feedback promptly instead of waiting out the +window, and the pump posts the Codex command for blocked rounds while keeping the CodeRabbit review +queued. `crq next` folds this into its `push`/`wait` answer for you. + +## User-Facing Updates + +Do not send heartbeat updates while a loop is simply waiting, and do not narrate repeated stderr +lines. Report a real state change or action: a review fired, findings returned, a push, convergence, +a timeout or unexpected failure, a rate-limit window first discovered or materially changed, a +network outage or recovery, or when the user asks. If the only new information is elapsed time on the +same wait, stay silent. ## Feedback @@ -188,7 +125,8 @@ Use this when you only need current findings and do not want to trigger a new re crq feedback "$REPO" "$PR" ``` -The output includes inline comments, GitHub review-thread IDs, collapsed/outside-diff review-body +`crq next` already embeds the current findings in its `fix` action, so reach for `crq feedback` only +when you want a snapshot without asking what to do about it. The output includes inline comments, GitHub review-thread IDs, collapsed/outside-diff review-body findings, prompt-block findings, Codex issue-comment findings, severity, path, line, source URL, commit, and bot. @@ -210,20 +148,26 @@ resolvable `thread_id`. After fixing a finding that has a `thread_id`, resolve that thread **on GitHub**: ```bash -crq resolve "$REPO" "$PR" --thread "$THREAD_ID" +crq resolve "$THREAD_ID" +crq resolve PRRT_one PRRT_two PRRT_three # resolve a whole round in one call ``` +Thread IDs are globally unique, so no repo or PR is needed. Pass every addressed thread to one +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. For a finding you are **not** addressing, record why instead of leaving it silently open: ```bash -crq decline "$REPO" "$PR" --thread "$THREAD_ID" --reason "why this is declined" +crq decline "$THREAD_ID" --reason "why this is declined" ``` -This replies on the thread with your reason and leaves it unresolved (add `--resolve` to also close it -as "won't fix"), so the next reviewer and CodeRabbit can see the decision rather than an ignored finding. +This replies with your reason and resolves the thread. crq reads GitHub's resolution state, so a +thread left open keeps its finding actionable and `crq next` would repeat `fix` forever. The +disagreement is not lost: if the bot contests the decline, crq re-surfaces that reply as its own +finding. Pass `--keep-open` to leave it unresolved deliberately. ## Fleet Auto-Review @@ -236,13 +180,13 @@ crq autoreview --no-incremental ``` Run exactly one long-lived autoreview daemon. If it is already active, do not stop, restart, or -duplicate it for a manual PR loop. `crq loop` and fleet autoreview use the same account-wide, -idempotent queue entry: after a push, autoreview may enqueue the new head first and the explicit loop -simply re-attaches (or vice versa). Neither path should post a direct CodeRabbit trigger. +duplicate it for a manual PR loop. `crq next`, `crq loop` and fleet autoreview all use the same +account-wide, idempotent queue entry: after a push, autoreview may enqueue the new head first and +your call only re-attaches (or vice versa). No path should post a direct CodeRabbit trigger. For an intentionally low-risk PR that has already had enough local review, add `` to the PR body before creating it. The marker is hidden in rendered -Markdown and prevents only fleet auto-review; an explicit `crq loop` still reviews the PR. +Markdown and prevents only fleet auto-review; an explicit `crq next`/`crq loop` still reviews the PR. ## Optional Local Preflight @@ -252,7 +196,7 @@ If the official CodeRabbit CLI is installed, agents can run a normalized local p crq preflight --type uncommitted ``` -Use that only to review local git changes before pushing. It does not replace `crq loop`, which +Use that only to review local git changes before pushing. It does not replace `crq next`, which coordinates queued GitHub PR review triggers and extracts GitHub PR feedback. ## Maintenance Commands