Add a live web dashboard, and three settings the fleet had no way to change - #62
Open
kristofferR wants to merge 68 commits into
Open
Add a live web dashboard, and three settings the fleet had no way to change#62kristofferR wants to merge 68 commits into
kristofferR wants to merge 68 commits into
Conversation
…change crq's only dashboard was a Markdown GitHub issue re-rendered after every state write. It answers "what is the queue doing" and nothing else: it cannot be clicked, and every setting behind it lives in an env file on whichever machine happens to run the daemon. `crq serve` starts a Go server with an embedded React SPA. State pushes over SSE on Rev change and countdowns tick client-side between pushes, so the page is live without polling. The expensive per-PR GitHub read is a second layer, fetched on open and cached by head, so the cheap state layer always renders instantly and a GitHub failure degrades one card instead of the page. Three settings move into shared state, each with a WriterCaps bump so a host running an older binary can be NAMED rather than silently ignoring the record: reviewers set REPO --no-primary CodeRabbit does not review this project repos add|remove REPO whether crq reviews it at all cost REPO PR what one more round there would cost The primary switch is the one with teeth. It reads as one more way a primary review cannot arrive, so every rule that already handles "no review is coming" handles it — and resolves before the slot, quota and pacing gates, which is what stops a private repository crq never fires on from queueing behind one it does. It also drops the primary from the effective required set, since a reviewer that does not run cannot gate, and is refused when nobody else is required. Enrollment precedence is the whole of that feature. CRQ_EXCLUDE wins over everything, because it is a per-host kill switch and the machine that has one usually has a reason the fleet does not know. Otherwise a record wins in BOTH directions: an Off switch whose only effect is to tell you which file to edit on another machine is not a switch. A record that turns off a repository a host's CRQ_REPOS still lists reports .env_conflict rather than letting the file and the fleet disagree in silence. Cost estimates are ranges, never a figure. Bytes-per-changed-line was measured across this repository's history at seven diff sizes and varies nearly 3x, so a single confident number would be the one output guaranteed to be wrong. Under about 85 changed lines the whole band fits below Macroscope's 10 KB minimum and the answer is exact. Each reviewer keeps its own .basis sentence, an unknown reviewer is Unknown rather than $0.00, and .prices_checked_at travels with it. A fire log lands with it: timestamps only, rolling two weeks, written in the same CAS as the fire it records. crq already recognised CodeRabbit's weekly fair-use throttle when the bot announced it, but recognising it is recognising an ~80% throughput collapse after the fact. The log forecasts it — and refuses to state a weekly count it lacks the history for, since an empty log reading as a quiet week is the exact mistake worth preventing. The activity feed is derived by diffing state revisions and says so: it starts when the server starts, and cannot see a change that appears and reverts between two polls. It is not an audit log and does not pretend to be one. internal/serve/dist is committed because go:embed cannot compile without it. Ref #61
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a new ChangesBackend platform
Estimated code review effort: 5 (Critical) | ~180 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant Server as serve.Server
participant Loader as crq.Service
participant Store as state.Store
Browser->>Server: GET /api/snapshot / SSE /api/events
Server->>Loader: Load()
Loader->>Store: read State
Store-->>Loader: State, Revision
Loader-->>Server: State
Server->>Server: BuildOverview + BuildFleet
Server-->>Browser: Snapshot JSON / SSE frame
Browser->>Server: POST /api/action/{action} (X-CRQ-Dashboard)
Server->>Server: addressedHere() auth check
Server->>Loader: Actor method (e.g. SetReviewers, SetEnrollment)
Loader->>Store: CAS Update(State)
Store-->>Loader: new State
Loader-->>Server: ActionResult
Server-->>Browser: snapshot / impact
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…drops Three findings from a local CodeRabbit preflight, all in the dashboard's front end. The confirmation dialog was a div. It now says so — role, aria-modal and a label — and keeps focus inside it: Tab wraps, Escape cancels, and focus goes back to whatever opened it, so cancelling a hold does not throw a keyboard user to the top of the list. The reason field's autoFocus is gone because the trap already focuses the first control, and two things claiming first focus is a race. The repository picker gets the same semantics and an Escape handler, but deliberately no Tab trap: it is a long scrolling list, and trapping Tab in it would strand somebody who tabs past the last row. The event stream retried every three seconds for ever. It now doubles to a thirty-second ceiling and resets on a successful open, so a server that is down for an hour is not asked twelve hundred times, while the common case — a restart lasting a second — still reconnects immediately. The cost card printed a dangling em dash when it had no error text to show.
`crq serve` had to be started by hand and died with the terminal that started it, which is a poor property for the thing you look at to find out whether the fleet is healthy. `crq serve install` writes a systemd user unit, or a launchd agent on macOS, and starts it — the same shape as `crq autofix install`, deliberately much thinner. That one bakes the whole fleet configuration into the unit because a fix session's behaviour has to be pinned at install time. The dashboard reads the same config file every other command reads, so its unit carries a path and nothing else: editing ~/.config/crq/env then changes the dashboard by restarting it rather than by reinstalling it. --dry-run prints the plan and writes nothing, so the unit can be read before it exists. A start that fails leaves the unit file in place and says which command to run by hand, rather than reporting a dashboard that is not up.
The settings that govern a whole fleet lived in ~/.config/crq/env on whichever machine happened to run the daemon. Changing one meant editing a file per host and hoping they agreed — and the dashboard could only display them. They now have a record in shared state, and reading one is three layers, least specific first: this host's env, then the fleet record, then a repository's own override. Every field is optional and absent means "keep using env", so a fleet that never writes a record behaves exactly as it did. The view reports a SOURCE per setting, because "this value is env" and "this value is the fleet's" look identical until you try to change one for everybody. Editable: the default reviewer set, the pacing floor, the weekly fair-use threshold, and whether a repository with no explicit switch may be fixed. A fleet save is not a per-repo save, and the UI stops pretending otherwise. It reaches every repository that has NOT overridden the setting, so both the CLI (--dry-run) and the dashboard ask the server what the change would do first, and the answer is the confirmation's entire body: affects 7 repositories following the fleet; 19 completed round(s) would be reopened and reviewed again (2 with their own reviewer override are unaffected) That number is the one worth seeing before clicking: a completed round is the "this head was reviewed" marker, and requiring a reviewer it never had makes the marker wrong, so the round is reopened and the metered ones spend the allowance again. WithFleet deliberately does not gate on UpdatedAt. A proposed record built for a preview carries no timestamp, and short-circuiting on that made every preview answer "nothing would change" — the one answer a preview must never give.
One watcher handles every repository, so every fix session ran with the same model, the same effort and the same attempt budget — the ones baked into the wrapper when autofix was installed. That is the wrong grain: a Go service worth a slow careful model and five attempts sits next to a docs repository where a fast one and a single attempt is the right trade. Model, effort, an extra standing prompt, the attempt budget, the fork permission and the skipped authors are now recorded per repository, over a fleet default, over this host's env — the same three layers the fleet settings use, and the view names which one answered for each setting. The mechanism is the interesting part. The watcher's argv is fixed when it starts, so per-repository values cannot travel in it. `crq autofix install` now writes TWO scripts: the wrapper that starts the watcher, and a session script the watcher runs per pull request. The session's ENVIRONMENT is built per dispatch, so CRQ_FIX_MODEL / CRQ_FIX_EFFORT / CRQ_FIX_PROMPT reach it there. An unset variable adds no flag at all rather than an empty one, which every agent rejects differently and none ignores. A session script from an older install simply ignores them, so reinstalling autofix is what turns this on. Reading the prompt in the session rather than the wrapper is a second win: editing it takes effect on the next session instead of the next restart. The AGENT stays fleet-wide, and the UI says so rather than offering a control that would do nothing. Switching between claude and codex is a different command line, not a different flag, and a per-repository agent would mean the watcher could not know what it was about to run until it ran it. Values that would reach a command line are validated here instead: an unknown effort is refused, because a session that dies on its first argument is a fix that silently never happens. So is a standing prompt over 4000 characters — it is appended to every session in the repository. Emptying every field clears the record rather than leaving one that overrides nothing, and the same now holds for the fleet record.
The Bots page listed four reviewers with no way to tell whether any of them was running, and no way to turn one off. Both halves were wrong in a way that hid the same fact: CRQ_COBOTS is unset on this fleet, so it defaults to all three co-reviewers, and nothing on the page said so. BotCard.Enabled was hardcoded true, because the list was built from the enabled reviewers — a set that cannot contain a disabled one. It is now built from the whole registry, so a bot crq knows how to drive gets a card whether it runs or not. A page that lists only the enabled ones cannot answer "why is Bugbot not reviewing this", and cannot offer the switch that would change the answer. The switch writes the fleet's whole co-reviewer list, because that is what the setting is — "these bots run", not a flag per bot. The primary has no switch: it is not a co-reviewer, and turning it off is a per-repository decision. Two bugs behind it: The dashboard's reviewer resolution took a repository's override alone, so it kept answering with this server's startup environment however the fleet had been configured. It takes the state now, which is where the fleet defaults live. The impact preview compared only the REQUIRED set, so turning a bot off reported "nothing would change" while changing which reviewers run on seven repositories. Running and gating are separate questions and both are changes: turning a bot off stops its findings arriving at all, which is not the same as it no longer holding the round open. The reopened count asks the same pair of questions reopenForChangedReviewers does, or it under-reports the consequence it exists to warn about — 19 rounds, in the case that found this.
Turning a bot off was a labelled button reading "Runs" or "Off", the fleet reviewer defaults were bare checkboxes, and the repository panel already had a proper switch. Three controls for the same decision, and the two new ones were the worse two. Toggle moves to ui.tsx and every on/off now uses it. It also carries its own title rather than hardcoding one about the primary reviewer, since "why can I not change this" is specific to whatever is locked — and gains role="switch" with aria-checked, which the button it replaced never had.
…rner The per-repo Reviewers card asks two questions of every bot — does it run, and does convergence wait for it — as two labelled toggles. The Bots page asked half of one, as a switch floating in the card header next to the "last seen" pill, where it read as a status rather than a control. It now has the same pair, worded and ordered the same way, and applies the same two rules: requiring a bot turns Runs on, and turning Runs off drops the requirement. Same questions at a different scope, so they should not look like different questions. The header goes back to being state alone.
The bundles are content-hashed, so a name that resolves at all resolves to the same bytes for ever. The page that NAMES them is not: it is served from the same fixed path every time, and a browser holding a cached copy keeps asking for the bundle it was built against — so a restarted server serves new assets nobody ever requests, and the dashboard silently stays a version behind while reporting a current revision in its own header. Hashed assets are now immutable for a year; everything else, index.html included, is no-cache.
I put Runs and Required toggles on the Bots page. They did not belong there. Which bots run is a property of a repository — its Reviewers card — or of the fleet default under Settings, and offering the same switch a third time meant three places to look for one answer, in the one place that had no business holding it. The page now answers what those two cannot: what IS this bot, what does it cost, and is it actually set up. Each card carries the vendor's own pitch and pricing in plain terms, what installing it involves, and links to sign up and to the docs. The descriptors live in dialect with every other bot string, since that is the one package allowed to know a bot by name; none of it affects a decision crq makes. Setup status is the honest part. crq cannot ask any vendor whether you have an account — none of them offers that — so it reports only what it has SEEN the bot do here: working, quiet, not verified, not enabled. "Enabled" and "working" are different claims and merging them hides the case that matters. CRQ_COBOTS defaults to all three co-reviewers, so a bot nobody signed up for reads as configured while reviewing nothing, and crq keeps posting a trigger comment nobody answers. That case now says so on its own card. No referral links, and the page says so rather than staying quiet about it.
The Bots page said Bugbot was working on a fleet that has never used it. It was reading CoBotRound.CommandedAt — the timestamp of crq POSTING "bugbot run" — as though it were the bot doing something. Every field in that record is crq's own bookkeeping: what it posted, and the claim it took before posting. None of them says a bot answered, so a bot with no account behind it looked identical to one working perfectly. crq asks, records that it asked, and nothing answers. CoBotRound gains AnsweredAt, stamped where crq has already paid for an observation and engine.CoReviewedHead says the bot produced head evidence — a review, a clean summary at the SHA, a completed check run. It only moves forward, so re-observing an old answer does not make a dormant bot look fresh. The guide now separates the two and says which it means. The status worth having is the one this makes possible: ASKED AND NEVER ANSWERED, which is the actual shape of an enabled-but-unconfigured bot, and which no amount of trigger bookkeeping could ever have shown. That card offers the setup steps and points out that every trigger crq posts for it is a comment nobody reads. The log is written only as rounds are observed, so it is empty right after an upgrade — and "nothing recorded" then means "not looked yet", not "never answered". Claiming the second would accuse a working reviewer of being unconfigured on the strength of a field introduced minutes ago, so co-reviewers stay "not verified" until the log has anything in it at all.
…m editable Every setting lived in one machine's env file, so the dashboard reported "env" beside all of them. True, and useless: it told you the value came from a file you could not see, on a host that might not be the one you were looking at. Three parts. `crq fleet adopt` copies this host's settings into the shared record, so they become the fleet's answer and every host reads the same one. It takes only what CAN be fleet-wide. Identity — which repository holds the queue, which ref, which issue — is reported as skipped, because a dashboard writing to that ref cannot move it without cutting the branch it sits on. Per-host values (paths, this machine's name, the fix agent) are skipped for the plainer reason that recording one machine's answer would break the others. A value equal to the built-in default is skipped too: recording one would pin today's default, invisibly, and a later crq could never improve it. Any setting is now fleet-settable, without each one needing plumbing of its own. FleetDefaults carries a raw key/value map, and applying it RE-PARSES the merged environment rather than patching fields — so a duration string becomes a duration the same way whichever layer supplied it, and the two paths cannot drift. LoadConfig splits into gather-then-BuildConfig to make that possible. Keys with a typed home (reviewers, pacing, weekly limit) route there instead, so one setting never lives in two places with one of them shadowed. The settings page lists all 32 with their effective value, the layer that decided it, and — where a record is overriding this host — what the host's own file still says. Identity and per-host rows are shown but locked, with the reason on the row: a disabled control that does not say why is worse than none. Two labelling bugs fell out of this. "env" was reported for values nothing configured, sending a reader to look for a line that was not there; default is now its own answer. And a setting adopted into its typed home kept reporting "env" afterwards, which is the exact mislabel this whole change exists to remove. Also: resolving a reviewer list now deduplicates, since "codex" and "chatgpt-codex-connector[bot]" name one bot and storing it twice rendered it twice.
`crq autofix install` wrote two bash scripts — a wrapper that started the watcher, and a session script that assembled the agent's command line. Three things then had to agree about the configuration, and two of them were generated text on disk that no test ever ran. A setting added to crq reached a fleet only after every host reinstalled, and nothing said which hosts had not: the per-repository model and effort landed a commit ago and were being ignored on both hosts, silently, because neither had a session script that knew to read them. The unit now runs `crq watch -- crq fix-session`. Same binary twice: one starts the watcher, one runs each session. The agent, its arguments and the prompt file travel in the unit's environment, so the install writes exactly one file that is not a unit — the prompt — and a setting reaches a host when its binary does. Building the agent's command line moves into Go, where it is testable: the argv builder now has the model and effort cases the shell version could not have, including the one that matters — an unset value adds no flag at all rather than an empty one, which every agent rejects differently and none ignores. It execs the agent rather than shelling to it, so the process the watcher supervises IS the agent: killing a session kills the agent, and the exit status is the agent's own rather than a shell's approximation of it.
…rom here autofixCanAuthenticate asks whether the SERVICE will have a GitHub credential, which is deliberately not the same question as whether this shell has one. On one host it cannot answer: gh keeps its token in the macOS login keychain, readable by a launchd agent in the GUI domain and not by an SSH session. Over SSH there, `gh auth status` names the account and calls the token invalid — which is also exactly what a genuinely expired token looks like. Since the two are indistinguishable from outside that session, the escape hatch is an explicit --skip-auth-check rather than a guess. An operator typing it has made a claim, which is not the silent nothing the check exists to prevent, and the flag says in one line what it is for.
Every queue and in-flight row read "coderabbit-queue#141" and nothing else, so the dashboard was a list of ticket numbers: finding the one you cared about meant opening them. The title is recorded on the round. The scan that finds a pull request already has it, so this costs no request — and a round already at the current head is never a candidate, so it would never be written again and would read as a bare number for ever. One converging write fixes that: once every round has its title nothing changes and the CAS reports no change. Stale after a rename, which is a fair price for a list that says what it is listing. With titles there is something to search, so there is now a search box and filter chips (all / in flight / queued / held / fixing / needs attention). Filtering is client-side because the whole snapshot is already in the page: it costs nothing and cannot go stale against the tables it narrows. A narrowed count reads "3 of 9" rather than "3", since a filtered table that says 3 reads as a fleet that suddenly has less work in it. Two more gaps from the mockups, both about a page that had gone read-only: A pull request opened by link could not be held or cancelled. Those actions existed only as hover buttons on an Overview row, which is not where you are when you have just read its findings. Both are on the PR page now, with the consequence stated — cancelling a fired round says that it releases the slot. The fix-session card said "attempt 2", which tells you nothing about whether that is nearly the last one. It now carries the repository's attempt budget, how many findings the session set out to fix, and where its log is going.
Two gaps the mockups had and the implementation did not. Every needs-attention item described a problem and left you to find the page that fixes it. They now carry a link: a broken host opens the host list, a stranded reservation opens the pull request, an expired leader opens setup, the fair-use warning opens fleet settings. An item that only describes a problem is a notification; one that points at the thing that fixes it is a control. A hold could only be placed from an Overview row — from the one page that shows every repository at once. So the answer to "stop reviewing this while I rework it" was to go and find it in a list of everything. Holds for a repository now live on that repository's page, with the reason each carries, an Unhold beside each, and a form to place one. It also says how many are held elsewhere, since a repository page that shows two holds should not read as a fleet with two.
Every question about a fleet turns into a per-host question, and crq could only answer for whichever machine you happened to be asking from. "Is claude installed" has a different answer on each host — and on any one host, a different answer again between your shell and the service, which is the whole failure: a tool installed for you and invisible to the daemon looks fine everywhere except in the fix session that needs it. Each host now reports itself into shared state: its crq version, what that binary understands, the services running there, and the tools it can reach. Reported, never inferred — nothing may write a record for another machine, because a host that has stopped reporting is exactly the one whose last claim about itself should stop being trusted. A stale record is kept and marked rather than dropped: "atlas last said it had claude, two days ago" beats an empty row, and the silence is itself the finding. The PATH probed is the reporting process's own, so a daemon reports the PATH a fix session inherits. That is the number that decides whether the agent starts. Setup gains the tool × host matrix and a ready / needs-attention / optional summary, so the page opens with a verdict rather than a list to count. The solver card gains the same inventory as capability beside its policy: a repository could be set to an agent no host can run, and nothing said so. Reporting writes nothing when nothing changed and the record is still fresh, so a daemon polling every minute does not bump the state revision every minute and make the activity feed report a fleet that is constantly doing something.
Three smaller mockup gaps. Each co-reviewer's trigger mode, self-heal grace and command are settings like any other, but were reachable only by editing an env file. They are generated from the registry rather than listed, so adding a co-reviewer adds its settings too — a hand-written catalogue silently stops covering the fleet it describes. A trigger that is not one of the three modes is refused: it would be accepted, written for every host, and then ignored. A fleet default now names the repositories it does NOT reach, and links to them. The count alone was a number you could not act on, and "affects 7 repositories" says nothing about which two are exempt or why. The solver card shows, per host, whether the configured agent is actually there — capability beside policy. A repository could be set to an agent no machine can run and nothing said so. A host that has never reported reads as unknown rather than missing, because blaming a machine for crq's own blind spot is worse than admitting the blind spot. Also fixes a bug the host table found in itself: two crq services on one machine each reported only their own role, so the autofix watcher and the review daemon took turns overwriting each other and the table showed whichever wrote last as the only thing running there. Roles merge; they are dropped only when the whole record ages out.
Two mockup gaps at opposite ends of the product. Adding a repository is the one click here that can spend real money: one with a dozen open pull requests becomes a dozen metered reviews on the next pass, and the button said "Add". It now prices the backlog first — open pull requests, how many would actually be enqueued once the skip rules apply, what the gap is for, and the range that would cost. Enrolling imageoptim, for instance, reads "would enqueue 8 of 8 open pull request(s) on the next pass — roughly $9.50–$28.80". Nothing is written until that has been shown. It prices the first 25 and says so rather than making a dialog nobody waits for, and a price it could not work out leaves the dialog up saying so instead of quietly becoming a free-looking Add. The other end: a fleet with nothing enrolled landed on an empty Overview, which says the queue is idle — indistinguishable from a queue that works and has nothing to do. It now gets a page that says what crq is for in three sentences (the metered lane, the free one, and fixing), and a checklist that answers itself from live state: whether the queue has a home, whether a daemon holds the lease, how many reviewers crq has actually SEEN work here as against how many are merely enabled, whether a fix agent exists. Derived, not a mode: it renders when nothing is enrolled and nothing has ever been queued, and stops the moment either becomes false. There is no first-run flag to get stuck on.
A converged pull request said "converged" in a pill and nothing else, so the one question it raises — is there anything for me to do — was answered by absence. It now leads with the verdict, which reviewers actually answered, and what happens next: nothing, merge when ready, and pushing another commit enqueues a fresh round. A converged round records that THIS head was reviewed, not that the pull request is finished, and reading it as the second is how a head goes out unreviewed. The bot guide's cost line now carries the date its prices were checked. An undated price is a price that has quietly stopped being true, and CodeRabbit's are already roughly double the figures widely cited a year ago. And a bot crq has not seen working gets a suggestion when there is evidence for one — right now: "the codex CLI is on cachyos, so the account behind it is probably already yours". That is the honest signal available, and it covers both cases worth a nudge: a bot switched off that you evidently have an account for, and one switched on that has never answered, where a CLI on a host says the account is fine and the setup is not. No evidence, no badge — a suggestion with no stated criterion is an advertisement.
Unenrolling asked for a reason and told you nothing about consequences. It now states the contract: no new rounds are enqueued, the ones already in flight finish (named, with a count, and where to cancel them sooner), and every setting is kept so turning it back on restores them rather than starting over. Setup gains the two summaries the mockup carries. Review bots, with what crq has actually seen each one do — the distinction that matters, since enabled and working are different claims. And repositories, which flags the ones still enrolled by this host's env file, because those are exactly the ones the other hosts decide for themselves, and points at the command that fixes it.
The mockups' signature affordance: a gear on any queue or in-flight row opens that repository's reviewers where you noticed you wanted to change them, rather than sending you to find it in a list of every repository. Deliberately only the reviewers, not a second settings page. A slide-over that grows into one is two places to look for one answer, which is the mistake the Bots page already made once — so it carries the two toggles and a link to everything else. It says what saving would DO before you save, in the terms that matter here: which bots would be newly enabled, and how many rounds already in flight would be reconsidered at their current heads. That is a consequence, not a preference. Missing tools on the Setup page now carry the command that installs them, and the reason installing is not enough: the service gets its own PATH, so a tool that works in your shell is still invisible to the thing that needs it.
Every other crq service reports its role, so the host table named the review daemon and the fix watcher and stayed silent about the one machine you are certainly talking to — the one serving the page you are reading it on.
kristofferR
marked this pull request as ready for review
July 28, 2026 03:19
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71a3f15234
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
CRQ_COBOTS defaulted to every co-reviewer in the registry, so a fresh install asked Bugbot and Macroscope on every round of every repository — posting a trigger comment, waiting out a grace period, and getting nothing, because nobody had an account for either. A co-reviewer is opt-in now. Anything named in CRQ_REQUIRED_BOTS is still enabled, since requiring a reviewer crq never asks would hang the round forever. The repository page marks a bot crq has never seen work there, and sorts the proven ones first. It still offers it: a bot cannot produce evidence until it is asked, so hiding the unproven ones would make the first one impossible to turn on. But "enabled" and "works" are different claims and the page now says which it means. Two bugs fell out of this. Narrowing a reviewer set reopened finished rounds. Removing two co-reviewers from a fleet default proposed re-reviewing SEVENTEEN completed rounds — that many metered reviews, against a shared allowance, to reach the answer already on record. Only ADDING a reviewer can invalidate a round: one that converged did so with more evidence than the new configuration asks for. Narrowing is free now, and the impact preview asks the same question so it stops warning about work that will not happen. And crq recorded a co-reviewer's answer only on the reviewing sweep, so a round that never fires — because the account is blocked, or the primary does not run there — was observed on the fire path and nowhere else. A co-reviewer could review it every time and crq would never notice. That is exactly what made a working Codex read as "never answered": it reviewed #58 twice today while the Bots page called it unverified.
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fa867cb0d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+573
to
+576
| case "CRQ_REQUIRED_BOTS": | ||
| change.Required = splitCommas(value) | ||
| if clear { | ||
| change.Required = nil |
There was a problem hiding this comment.
When the settings UI or crq settings ... --clear unsets CRQ_REQUIRED_BOTS, this branch changes Required to nil, but applyFleetChange defines nil as “leave this field alone.” The call consequently returns success while the recorded fleet override remains active; CRQ_COBOTS has the same behavior in the adjacent branch. Clearing these typed settings needs to reset their SetRequired/SetCoBots flags rather than encode the request as an omitted change.
Useful? React with 👍 / 👎.
Comment on lines
+417
to
+419
| "CRQ_FIX_AGENT": plan.Agent, | ||
| "CRQ_FIX_ARGS": strings.Join(plan.AgentArgs, " "), | ||
| "CRQ_FIX_PROMPT_FILE": plan.Prompt, |
There was a problem hiding this comment.
When an --agent-args value contains whitespace inside one argument, such as a prompt or config path, joining the parsed argv with spaces discards its boundaries; FixSession later calls SplitArgv and turns that one argument into several. This regresses the previous generated invocation, which quoted every argument independently, and can make every dispatched agent invocation fail or receive different options. Store a lossless encoding or re-quote each argv element before placing it in CRQ_FIX_ARGS.
Useful? React with 👍 / 👎.
Owner
Author
|
@codex review |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1a42d621e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a1b38f384
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98dc0cd3f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
|
@codex review |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/crq/enrollment.go`:
- Around line 119-129: Remove the DryRun-specific early-return branch in
SetEnrollment so explicit enrollment changes always use the normal persistence
path, including s.store.Update. Preserve the existing claimedTriggerRepo
validation and ensure the returned EnrollmentView reflects the persisted record,
matching ClearEnrollment behavior.
In `@internal/serve/fleet.go`:
- Around line 597-612: Build the reviewer name set in the repo aggregation loop
from the effective reviewers returned by botsFor(repo), rather than only
rv.CoBots and rv.Required. Ensure fleet-default co-reviewers are included in
named so repoCount, repoBots, and repoRequired are populated for them, while
preserving normalization and enabled-reviewer filtering.
In `@web/src/AddRepo.tsx`:
- Around line 17-28: Update EnrollImpactCopy so a preview with an enrollment
failure is not treated as in progress: detect the enroll error state before the
!preview.impact loading branch and render the appropriate enrollment-failure
copy while the dialog is idle. Preserve the existing preview-failure message and
loading behavior for previews without an error.
In `@web/src/pages/ReposPage.tsx`:
- Around line 380-385: Update the payload construction in ReposPage and
QuickSettings so required and cobots apply the same configurable-name treatment,
and identify names of already-enrolled non-configurable reviewers that will be
dropped. Include those dropped names in ReposPage’s Confirm body alongside
newlyOn and in QuickSettings’ “Saving would…” banner, while preserving the
whole-intended-set behavior and existing toggle state handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1d40a419-dfc3-4aef-b6b7-f80fb5a2390e
⛔ Files ignored due to path filters (10)
internal/serve/dist/assets/BotsRoute-CR1OFTxv.jsis excluded by!**/dist/**internal/serve/dist/assets/OverviewRoute-DmFbobmZ.jsis excluded by!**/dist/**internal/serve/dist/assets/PRRoute-DWpgP0X0.jsis excluded by!**/dist/**internal/serve/dist/assets/ReposRoute-BsgfE0D7.jsis excluded by!**/dist/**internal/serve/dist/assets/SettingsRoute-D6u2nMwT.jsis excluded by!**/dist/**internal/serve/dist/assets/SetupRoute-H29-Z2e-.jsis excluded by!**/dist/**internal/serve/dist/assets/index-Dl6Lk7B6.jsis excluded by!**/dist/**internal/serve/dist/assets/ui-BAhA1DbR.jsis excluded by!**/dist/**internal/serve/dist/assets/useOperation-DB3gaLSh.jsis excluded by!**/dist/**internal/serve/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (18)
internal/crq/enrollment.gointernal/crq/enrollment_test.gointernal/crq/repoconfig.gointernal/crq/repoconfig_test.gointernal/crq/service.gointernal/serve/events.gointernal/serve/events_test.gointernal/serve/fleet.gointernal/serve/server.gointernal/serve/server_test.goweb/src/AddRepo.tsxweb/src/PRDetail.tsxweb/src/QuickSettings.tsxweb/src/ReviewFixes.test.tsweb/src/SolverEditor.tsxweb/src/client.test.tsweb/src/data/contracts.tsweb/src/pages/ReposPage.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
internal/crq/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/crq/**/*.go: Keepinternal/crqorchestration-only: wire packages and implement service flows, observation, feedback, configuration, calibration, preflight, and initialization without duplicating bot wording or decision rules.
Use the shared observe → decide → apply flow for both daemon and loop paths; do not create alternate fire or completion logic outside the designated engine functions.
Preserve loop exit codes:0for converged/skipped/held,10for findings, and2only for timeout; a hold is terminal and must not be reported as timeout.
Files:
internal/crq/enrollment.gointernal/crq/repoconfig_test.gointernal/crq/repoconfig.gointernal/crq/enrollment_test.gointernal/crq/service.go
internal/crq/service.go
📄 CodeRabbit inference engine (AGENTS.md)
crq/service.gois the only effects executor for CAS state writes andPostIssueComment;DryRunmust report without writing.
Files:
internal/crq/service.go
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: kristofferR/coderabbit-queue
Timestamp: 2026-07-29T03:56:48.137Z
Learning: Maintain the dependency direction `dialect ← engine ← crq`, `state ← crq`, `gh ← {state, crq}`, and `workspace ← crq`; do not introduce dependency cycles.
📚 Learning: 2026-07-27T01:11:18.244Z
Learnt from: kristofferR
Repo: kristofferR/coderabbit-queue PR: 57
File: internal/crq/feedback.go:91-92
Timestamp: 2026-07-27T01:11:18.244Z
Learning: When reviewing code in `internal/crq` that deals with bot configuration, keep the separation of concerns intact: `Config.isConfiguredBot(login)` should only be evaluated against the fleet-wide primary `Config.Bot`. Do not override or reinterpret `Config.Bot` based on repo-specific reviewer/co-reviewer data. If repo-added co-reviewers are involved, they must be handled via repo-derived bot sets (e.g., `Config.evidenceBots()`), and not via `isConfiguredBot`. `Config.ForRepo(...)` should adjust co-reviewer-derived fields like `CoBots`, `RequiredBots`, `Reviewers`, and `FeedbackBots`, but intentionally not change the meaning of `Config.Bot` used by `isConfiguredBot`.
Applied to files:
internal/crq/enrollment.gointernal/crq/repoconfig_test.gointernal/crq/repoconfig.gointernal/crq/enrollment_test.gointernal/crq/service.go
📚 Learning: 2026-07-27T01:11:20.071Z
Learnt from: kristofferR
Repo: kristofferR/coderabbit-queue PR: 57
File: internal/crq/repoconfig.go:163-183
Timestamp: 2026-07-27T01:11:20.071Z
Learning: In internal/crq, treat the “observe → decide → apply” effects restriction as applying only to effects produced by review decisions. Explicit operator mutation commands (e.g., Service.SetReviewers, Service.ClearReviewers, enqueue/cancel flows) are allowed to perform state mutations outside the standard apply path, so review logic should not incorrectly require them to follow observe/decide/apply.
For CRQ_DRY_RUN: it should suppress review requests and fire-record writes, but it must not make explicit configuration/mutation commands (e.g., `crq reviewers set`) silently succeed without persisting the requested changes.
Ensure Service.applyFire revalidates repository reviewer overrides just before posting, to protect against stale/changed overrides since earlier reads.
Applied to files:
internal/crq/enrollment.gointernal/crq/repoconfig_test.gointernal/crq/repoconfig.gointernal/crq/enrollment_test.gointernal/crq/service.go
📚 Learning: 2026-07-28T06:12:02.107Z
Learnt from: kristofferR
Repo: kristofferR/coderabbit-queue PR: 58
File: internal/crq/service.go:2113-2116
Timestamp: 2026-07-28T06:12:02.107Z
Learning: In the internal/crq package, treat `rate-limit-command` and `gate-repo` as host-local configuration (not fleet-owned settings). `Service.readQuota` may accept a fleet-derived `Config` only to use the fleet-owned `Scope` and `CalibrationTTL`. Ensure the calibration probe command and any gate-repository logic continue to use the host-local service configuration, not the fleet-owned configuration.
Applied to files:
internal/crq/enrollment.gointernal/crq/repoconfig_test.gointernal/crq/repoconfig.gointernal/crq/enrollment_test.gointernal/crq/service.go
🔇 Additional comments (23)
internal/serve/events.go (1)
65-242: LGTM!internal/serve/server.go (1)
254-258: LGTM!internal/serve/server_test.go (1)
63-80: LGTM!Also applies to: 403-403, 435-435, 457-457, 479-479, 492-540
internal/serve/events_test.go (1)
53-66: LGTM!web/src/client.test.ts (1)
4-4: LGTM!Also applies to: 73-89
web/src/data/contracts.ts (1)
248-248: LGTM!Also applies to: 418-420
web/src/PRDetail.tsx (1)
521-526: LGTM!internal/serve/fleet.go (3)
190-200: LGTM!Also applies to: 357-365
660-664: LGTM!
632-635: 🎯 Functional CorrectnessNo change needed.
CoReviewerByNameintentionally accepts either the co-reviewer config name or the GitHub login, so passing the currentBotCard.Loginis valid.> Likely an incorrect or invalid review comment.web/src/QuickSettings.tsx (1)
94-101: LGTM!Also applies to: 116-121
web/src/pages/ReposPage.tsx (1)
453-460: LGTM!Also applies to: 468-473, 622-622
web/src/ReviewFixes.test.ts (1)
74-74: LGTM!Also applies to: 99-99
web/src/SolverEditor.tsx (1)
105-133: LGTM!Also applies to: 166-184, 207-211, 227-231, 316-320, 330-334, 409-409
web/src/AddRepo.tsx (1)
76-81: LGTM!Also applies to: 282-284, 328-350, 436-440, 456-466
internal/crq/enrollment_test.go (2)
287-329: 🎯 Functional CorrectnessThis test locks in the disputed DryRun behavior.
This test encodes the same DryRun-does-not-persist behavior flagged as a major issue on
internal/crq/enrollment.go(lines 119-129). If that branch is fixed to persist under DryRun, this test's assertions (no revision change, no enrollment recorded) will need to be rewritten accordingly.
383-431: LGTM!Also applies to: 607-645
internal/crq/repoconfig.go (2)
26-37: LGTM!Also applies to: 98-143, 155-200, 489-508
144-154: 🎯 Functional CorrectnessNo change needed.
cfgForcurrently uses the same Fleet+repo-layering path as the reopen-diff baseline.internal/crq/repoconfig_test.go (1)
324-363: LGTM!Also applies to: 374-393, 416-422, 458-464, 503-512, 562-568, 591-596, 619-625, 697-703, 772-797, 844-850, 881-887, 920-926, 942-1090
internal/crq/enrollment.go (2)
1-49: LGTM!
130-161: LGTM!internal/crq/service.go (1)
40-42: LGTM!Also applies to: 330-333, 911-962
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cff57efec5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/crq/watch.go`:
- Around line 841-860: Update the clarification handling branch around
clarificationFromLog to check lost() before calling Hold or
stopDispatchForClarification. If the dispatch race was already lost, follow the
existing sibling-branch behavior and return without applying a hold or terminal
clarification stop; only the current token owner may act on the clarification.
In `@web/src/pages/ReposPage.tsx`:
- Around line 388-391: Preserve locked required gates when constructing the save
payload so unrelated reviewer changes cannot produce an explicit empty required
set. Update the required-gate filtering in web/src/pages/ReposPage.tsx lines
388-391 and apply the same fix to web/src/QuickSettings.tsx lines 54-55,
retaining locked/custom required names or blocking submission with an actionable
validation message when none remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ee41ab79-e534-41f8-8252-374181b17cea
⛔ Files ignored due to path filters (10)
internal/serve/dist/assets/BotsRoute-CWD5wL4D.jsis excluded by!**/dist/**internal/serve/dist/assets/OverviewRoute-MAD4oZKS.jsis excluded by!**/dist/**internal/serve/dist/assets/PRRoute-CQ4b0aFx.jsis excluded by!**/dist/**internal/serve/dist/assets/ReposRoute-Cl7o5Wlz.jsis excluded by!**/dist/**internal/serve/dist/assets/SettingsRoute-B_fX75XV.jsis excluded by!**/dist/**internal/serve/dist/assets/SetupRoute-CkrMbDau.jsis excluded by!**/dist/**internal/serve/dist/assets/index-CjwRQ2K3.jsis excluded by!**/dist/**internal/serve/dist/assets/ui-BhRdu4oK.jsis excluded by!**/dist/**internal/serve/dist/assets/useOperation-C7imuBXP.jsis excluded by!**/dist/**internal/serve/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (16)
cmd/crq/main.gointernal/crq/enrollment.gointernal/crq/enrollment_test.gointernal/crq/serveinstall.gointernal/crq/serveinstall_test.gointernal/crq/service.gointernal/crq/watch.gointernal/crq/watch_test.gointernal/serve/actions.gointernal/serve/fleet.gointernal/serve/server_test.gointernal/state/dispatch_test.gointernal/state/state.goweb/src/AddRepo.tsxweb/src/QuickSettings.tsxweb/src/pages/ReposPage.tsx
💤 Files with no reviewable changes (1)
- internal/serve/fleet.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
internal/crq/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/crq/**/*.go: Keepinternal/crqorchestration-only; it owns service wiring, observation, application of effects, configuration, feedback, and lifecycle commands, but not bot wording or decision logic.
Use the observe → decide → apply flow:observe.gois the sole GitHub observation/reduction point,internal/engineperforms pure decisions, andservice.gois the only effects executor;DryRunmust write nothing.
Only the apply layer may perform effects such as CAS state writes andPostIssueComment; no other orchestration path may post review commands.
Preserve frozen loop exit codes: 0 for converged/skipped/held, 10 for findings, and 2 only for timeout; a hold is terminal and must not be reported as timeout.
Files:
internal/crq/serveinstall_test.gointernal/crq/serveinstall.gointernal/crq/enrollment_test.gointernal/crq/enrollment.gointernal/crq/watch_test.gointernal/crq/watch.gointernal/crq/service.go
internal/state/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/state/**/*.go: Persist oneRoundper PR, one globalFireSlot, account quota, archive, and repository records; never delete rounds, and preserve unknown JSON members forState,Round, and every nested record.
Model round transitions as methods onRound; reject illegal edges, never delete rounds, and useawaiting_retryfor rate-limited requeues while preserving head, attempts, and history.
Files:
internal/state/dispatch_test.gointernal/state/state.go
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: kristofferR/coderabbit-queue
Timestamp: 2026-07-29T05:08:17.856Z
Learning: Respect the dependency direction `dialect ← engine ← crq`, `state ← crq`, `gh ← {state, crq}`, and `workspace ← crq`; dependencies must not form cycles.
📚 Learning: 2026-07-27T01:11:18.244Z
Learnt from: kristofferR
Repo: kristofferR/coderabbit-queue PR: 57
File: internal/crq/feedback.go:91-92
Timestamp: 2026-07-27T01:11:18.244Z
Learning: When reviewing code in `internal/crq` that deals with bot configuration, keep the separation of concerns intact: `Config.isConfiguredBot(login)` should only be evaluated against the fleet-wide primary `Config.Bot`. Do not override or reinterpret `Config.Bot` based on repo-specific reviewer/co-reviewer data. If repo-added co-reviewers are involved, they must be handled via repo-derived bot sets (e.g., `Config.evidenceBots()`), and not via `isConfiguredBot`. `Config.ForRepo(...)` should adjust co-reviewer-derived fields like `CoBots`, `RequiredBots`, `Reviewers`, and `FeedbackBots`, but intentionally not change the meaning of `Config.Bot` used by `isConfiguredBot`.
Applied to files:
internal/crq/serveinstall_test.gointernal/crq/serveinstall.gointernal/crq/enrollment_test.gointernal/crq/enrollment.gointernal/crq/watch_test.gointernal/crq/watch.gointernal/crq/service.go
📚 Learning: 2026-07-27T01:11:20.071Z
Learnt from: kristofferR
Repo: kristofferR/coderabbit-queue PR: 57
File: internal/crq/repoconfig.go:163-183
Timestamp: 2026-07-27T01:11:20.071Z
Learning: In internal/crq, treat the “observe → decide → apply” effects restriction as applying only to effects produced by review decisions. Explicit operator mutation commands (e.g., Service.SetReviewers, Service.ClearReviewers, enqueue/cancel flows) are allowed to perform state mutations outside the standard apply path, so review logic should not incorrectly require them to follow observe/decide/apply.
For CRQ_DRY_RUN: it should suppress review requests and fire-record writes, but it must not make explicit configuration/mutation commands (e.g., `crq reviewers set`) silently succeed without persisting the requested changes.
Ensure Service.applyFire revalidates repository reviewer overrides just before posting, to protect against stale/changed overrides since earlier reads.
Applied to files:
internal/crq/serveinstall_test.gointernal/crq/serveinstall.gointernal/crq/enrollment_test.gointernal/crq/enrollment.gointernal/crq/watch_test.gointernal/crq/watch.gointernal/crq/service.go
📚 Learning: 2026-07-28T06:12:02.107Z
Learnt from: kristofferR
Repo: kristofferR/coderabbit-queue PR: 58
File: internal/crq/service.go:2113-2116
Timestamp: 2026-07-28T06:12:02.107Z
Learning: In the internal/crq package, treat `rate-limit-command` and `gate-repo` as host-local configuration (not fleet-owned settings). `Service.readQuota` may accept a fleet-derived `Config` only to use the fleet-owned `Scope` and `CalibrationTTL`. Ensure the calibration probe command and any gate-repository logic continue to use the host-local service configuration, not the fleet-owned configuration.
Applied to files:
internal/crq/serveinstall_test.gointernal/crq/serveinstall.gointernal/crq/enrollment_test.gointernal/crq/enrollment.gointernal/crq/watch_test.gointernal/crq/watch.gointernal/crq/service.go
🔇 Additional comments (15)
web/src/QuickSettings.tsx (1)
34-35: Resync selections when the repository snapshot changes.
runsandrequiredare only initialized from props. An SSE update while this sheet is open leaves stale toggles and incorrect dirty state.internal/serve/actions.go (1)
180-184: LGTM!Also applies to: 212-216, 241-245, 308-326
internal/serve/server_test.go (1)
82-94: LGTM!Also applies to: 556-579
cmd/crq/main.go (1)
612-612: LGTM!internal/crq/serveinstall.go (1)
35-35: LGTM!Also applies to: 58-60, 70-105, 221-228
internal/crq/serveinstall_test.go (1)
54-63: LGTM!web/src/AddRepo.tsx (1)
26-33: LGTM!internal/state/dispatch_test.go (1)
4-4: LGTM!Also applies to: 82-100, 102-145
internal/state/state.go (2)
1-1: LGTM!Also applies to: 72-78, 126-146, 214-233, 297-324, 337-347, 413-427, 466-469, 587-590, 591-591, 625-628, 689-728, 738-757, 1247-1257, 1258-1261, 1262-1276, 1295-1295, 1321-1321, 1362-1420, 1424-1523, 2026-2053
1421-1423: 🗄️ Data Integrity & IntegrationNo change needed. Clarification is part of the existing dispatch claim, which moves with the current Round; the existing test already covers a fresh Round clearing the previous head’s termination on head replacement.
> Likely an incorrect or invalid review comment.internal/crq/enrollment.go (1)
153-166: LGTM!internal/crq/enrollment_test.go (1)
287-354: LGTM!internal/crq/service.go (1)
981-993: LGTM!Also applies to: 1496-1503, 1538-1552, 1808-1845
internal/crq/watch.go (1)
1238-1256: LGTM! Correctly guarded byMarkDispatchClarification's ownDispatch.Token != tokencheck, so a stale token cannot clobber a claim now owned by a different session.internal/crq/watch_test.go (1)
113-155: LGTM!
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/crq/watch.go`:
- Line 841: Update the clarification hold flow around the `question` check so
recording the clarification hold/stop and validating dispatch ownership occur in
one atomic `s.Hold` update. Do not rely on the separate `!lost()` heartbeat
check before calling `s.Hold`; ensure a watcher that loses the claim cannot
persist the hold.
In `@web/src/QuickSettings.tsx`:
- Around line 37-43: Update the serverRuns and serverRequired snapshot keys in
QuickSettings so they are order-independent, matching sameMembers by sorting the
arrays before joining with a delimiter. Keep the useEffect dependencies and
local state updates unchanged, so reordering reviewers or required entries does
not reset local edits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0cecf199-c012-4ced-82c1-345da9ea6b66
⛔ Files ignored due to path filters (10)
internal/serve/dist/assets/BotsRoute-B7QhQ3P_.jsis excluded by!**/dist/**internal/serve/dist/assets/OverviewRoute-DFFVH_z4.jsis excluded by!**/dist/**internal/serve/dist/assets/PRRoute-BLDvRKHU.jsis excluded by!**/dist/**internal/serve/dist/assets/ReposRoute-BCakgV0G.jsis excluded by!**/dist/**internal/serve/dist/assets/SettingsRoute-DSLvk3ko.jsis excluded by!**/dist/**internal/serve/dist/assets/SetupRoute-CXlpkG8V.jsis excluded by!**/dist/**internal/serve/dist/assets/index-OVu0mBoz.jsis excluded by!**/dist/**internal/serve/dist/assets/ui-BYzuZ4DM.jsis excluded by!**/dist/**internal/serve/dist/assets/useOperation-DCw1kDXQ.jsis excluded by!**/dist/**internal/serve/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (3)
internal/crq/watch.goweb/src/QuickSettings.tsxweb/src/pages/ReposPage.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
internal/crq/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/crq/**/*.go: Keepinternal/crqlimited to orchestration and wiring; it must not contain bot wording or duplicate engine decision rules.
Preserve frozen loop exit codes: 0 for converged/skipped/held, 10 for findings, and 2 only for timeout; holds are terminal and must not return timeout.
Files:
internal/crq/watch.go
🧠 Learnings (3)
📚 Learning: 2026-07-27T01:11:18.244Z
Learnt from: kristofferR
Repo: kristofferR/coderabbit-queue PR: 57
File: internal/crq/feedback.go:91-92
Timestamp: 2026-07-27T01:11:18.244Z
Learning: When reviewing code in `internal/crq` that deals with bot configuration, keep the separation of concerns intact: `Config.isConfiguredBot(login)` should only be evaluated against the fleet-wide primary `Config.Bot`. Do not override or reinterpret `Config.Bot` based on repo-specific reviewer/co-reviewer data. If repo-added co-reviewers are involved, they must be handled via repo-derived bot sets (e.g., `Config.evidenceBots()`), and not via `isConfiguredBot`. `Config.ForRepo(...)` should adjust co-reviewer-derived fields like `CoBots`, `RequiredBots`, `Reviewers`, and `FeedbackBots`, but intentionally not change the meaning of `Config.Bot` used by `isConfiguredBot`.
Applied to files:
internal/crq/watch.go
📚 Learning: 2026-07-27T01:11:20.071Z
Learnt from: kristofferR
Repo: kristofferR/coderabbit-queue PR: 57
File: internal/crq/repoconfig.go:163-183
Timestamp: 2026-07-27T01:11:20.071Z
Learning: In internal/crq, treat the “observe → decide → apply” effects restriction as applying only to effects produced by review decisions. Explicit operator mutation commands (e.g., Service.SetReviewers, Service.ClearReviewers, enqueue/cancel flows) are allowed to perform state mutations outside the standard apply path, so review logic should not incorrectly require them to follow observe/decide/apply.
For CRQ_DRY_RUN: it should suppress review requests and fire-record writes, but it must not make explicit configuration/mutation commands (e.g., `crq reviewers set`) silently succeed without persisting the requested changes.
Ensure Service.applyFire revalidates repository reviewer overrides just before posting, to protect against stale/changed overrides since earlier reads.
Applied to files:
internal/crq/watch.go
📚 Learning: 2026-07-28T06:12:02.107Z
Learnt from: kristofferR
Repo: kristofferR/coderabbit-queue PR: 58
File: internal/crq/service.go:2113-2116
Timestamp: 2026-07-28T06:12:02.107Z
Learning: In the internal/crq package, treat `rate-limit-command` and `gate-repo` as host-local configuration (not fleet-owned settings). `Service.readQuota` may accept a fleet-derived `Config` only to use the fleet-owned `Scope` and `CalibrationTTL`. Ensure the calibration probe command and any gate-repository logic continue to use the host-local service configuration, not the fleet-owned configuration.
Applied to files:
internal/crq/watch.go
🔇 Additional comments (2)
web/src/QuickSettings.tsx (1)
2-2: LGTM!Also applies to: 49-56, 63-63
web/src/pages/ReposPage.tsx (1)
366-369: LGTM!Also applies to: 390-390
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
crq servestarts a Go server with an embedded React/Vite SPA. State pushesover SSE on
Revchange and countdowns tick client-side between pushes, so thepage is live without polling. The expensive per-PR GitHub read is a second
layer, fetched on open and cached by head, so the cheap state layer always
renders instantly and a GitHub failure degrades one card rather than the page.
Alongside it, three settings move out of per-host env files and into shared
state, each with a
WriterCapsbump so a host on an older binary is namedrather than silently ignoring the record.
What's in it
Turn CodeRabbit off for one repository —
crq reviewers set REPO --no-primary.The mechanism is one line in the engine: it reads as one more way a primary
review cannot arrive, so every rule that already handles "no review is coming"
handles this. That path resolves before the slot, quota and pacing gates,
which is what stops a private repo crq never fires on from queueing behind one
it does. It also drops the primary from the effective required set — a reviewer
that does not run cannot gate — and is refused when nobody else is required.
Enrollment in shared state —
crq repos add|remove|default REPO, plus apicker in the dashboard over everything in
CRQ_SCOPE. Precedence is the wholefeature:
CRQ_EXCLUDEwins over everything (a per-host kill switch, and themachine that has one usually has a reason the fleet does not know); otherwise a
record wins in both directions, because an Off switch whose only effect is
to tell you which file to edit on another machine is not a switch. A record
that turns off a repo a host's
CRQ_REPOSstill lists reportsenv_conflictrather than letting the file and the fleet disagree in silence.
Cost estimates —
crq cost REPO PR, and a card on the PR page. Ref #61.Ranges, never a figure: bytes-per-changed-line was measured across this
repository's history at seven diff sizes and varies nearly 3×, so a single
confident number would be the one output guaranteed to be wrong. Under ~85
changed lines the whole band fits below Macroscope's 10 KB minimum and the
answer is exact. Each reviewer keeps its own
basissentence, an unknownreviewer is
unknownrather than$0.00, andprices_checked_attravels withit. Two payloads already sitting unparsed in the corpus are now read: Macroscope's
**Agent Credits:**and CodeRabbit'sisProUser/policyGuidance.A fair-use forecast — timestamps only, rolling two weeks, written in the
same CAS as the fire it records. crq already recognised CodeRabbit's weekly
throttle when the bot announced it, but recognising it is recognising an ~80%
throughput collapse after the fact.
CRQ_WEEKLY_LIMIT(default 60) sets thethreshold. It refuses to state a weekly count it lacks the history for — an
empty log reading as a quiet week is the exact mistake worth preventing.
Only enabled bots are shown. The dashboard rendered the fleet bot list on
every row, so a repo that does not run Bugbot still got a greyed-out Bugbot
mark on each of its PRs. Reviewer resolution is now passed in from
Config.ForRepo, so the dashboard cannot grow a second answer to "who reviewshere". That also fixed a real inconsistency where an overridden repo listed bot
logins and an inherited one listed short names.
An activity feed derived by diffing state revisions, which says so: it
starts when the server starts and cannot see a change that appears and reverts
between two polls. Not an audit log, and it does not pretend to be one.
Worth a careful look
web/adds a bun/Vite/React/Tailwind toolchain to a repo that had nobuild dependencies at all. That changes what
git clone && go buildmeans.internal/serve/distis committed —//go:embed all:distcannot compilewithout it. Your global gitignore's blanket
dist/was hiding it, so thereis an explicit negation in
.gitignore; git will not descend into an excludeddirectory to re-include files.
WriterCapsbumps in one change (1 → 3). Every one is additive andreports its own lagging hosts, but it is three at once.
targets, so a repo enrolled from the dashboard is searched on the next pass
rather than after a restart. A scope-wide host (
CRQ_REPOSempty) must notbe narrowed by records — that has its own test.
Still to do
crq serveis not a service yet; it dies on reboot.still fleet-level env only.
conversion and there is no capture of a check-run detail page with a billed
cost, so there is nowhere honest to spend them yet.
rendered against a real finding — no PR currently has any.
A local
crq preflight --type uncommittedwas started before the first commitand is still running against the 8k-line diff; its findings will land as
follow-up commits here.
Summary by CodeRabbit
crq serve,crq cost,crq fleet,crq solver,crq repos,crq reviewers, plus queuecrq prioritizeand session log tailing.