Make the reviewer configuration a value, not a field by kristofferR · Pull Request #51 · kristofferR/coderabbit-queue

kristofferR · 2026-07-26T16:22:52Z

Groundwork for roadmap item H (per-repo reviewer configuration), split out so the mechanical part
lands on its own and the feature that follows is small.

Why

Who reviews a PR was read straight off the Service:

for_, cb:=ranges.cfg.CoBots { ... }
dialect.BotSet(unionBots(s.cfg.FeedbackBots, s.cfg.RequiredBots))
engine.DecideFire(global, *next, obs.eng, now, s.policy())

That is fine while every repository has the same answer — and that is exactly the thing that has to
stop being true. "Which bots you want for which project" cannot be a property of the process.

What changed

The reads become functions of a Config value:

policy(), classifierCoReviewers(), coChecksRelevant(), codexRelevant(), coBotEnabled(),
coCommandFor(), coCommandBodies() are now Config methods.
observe() takes the configuration it should use as a parameter instead of reaching for the
Service's own.
evidenceBots() gives the union of feedback and required bots one definition. It had two copies in
Feedback, which had to stay equal — a bot crq waits for whose findings it does not surface hangs
the round forever.

No behaviour change. Every caller passes s.cfg, which is what each site read before. What
changes is that passing something else is now possible.

Zero remaining direct reads:

$ grep -rn 's\.cfg\.\(CoBots\|RequiredBots\|FeedbackBots\)' --include=*.go internal/crq/ | grep -v _test | wc -l
0

Verification

go test ./... -count=1 green across all six packages — including the replay suites, which drive the
fire, observe, completion and co-review paths this touches. gofmt -l . and go vet ./... clean.

A pure refactor's test is that the existing tests keep passing without being edited to suit it; the
only test changes here are the two observe(...) call sites gaining the parameter.

Ref #42

Summary by CodeRabbit

Bug Fixes
- Ensured review processing consistently uses the active configuration when evaluating bots, policies, reactions, co-reviewers, and adoption decisions.
- Improved evidence collection to include both feedback and required review bots.
- Prevented configuration mismatches from affecting queue progression and self-healing behavior.
Tests
- Updated replay tests and added coverage verifying configuration-isolated review processing.

Who reviews a PR was read straight off the Service: s.cfg.CoBots, s.cfg.RequiredBots, s.cfg.FeedbackBots, and a policy() built from them. That is fine while every repository has the same answer, and it is the one thing that has to stop being true — "which bots you want for which project" cannot be a property of the process. So the reads become functions of a Config value: policy, the classifier's co-reviewer list, the check-run and Codex relevance probes, the trigger command lookups, and the evidence set. observe takes the configuration it should use as a parameter rather than reaching for the Service's own. No behaviour change — every caller passes s.cfg, which is what it read before. What changes is that passing something else is now possible, and that the union of feedback and required bots has one definition instead of two copies that had to stay equal.

coderabbitai · 2026-07-26T16:22:58Z

Warning

Review limit reached

@kristofferR, you've reached your PR review limit, so we couldn't start this review.

Next review available in:45 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 76f894f2-f227-47e4-9898-af2fea4fb096

📥 Commits

Reviewing files that changed from the base of the PR and between 1ed0ed9 and 9853acf.

📒 Files selected for processing (1)

internal/crq/observe_config_test.go

📝 Walkthrough

Walkthrough

The refactor moves policy and bot-eligibility decisions onto Config, passes configuration explicitly into observation and adoption paths, updates feedback evidence extraction, and adjusts related tests and call sites.

Changes

Config-driven review flow

Layer / File(s)	Summary
Config policy and helper contracts `internal/crq/state.go`, `internal/crq/reviewers.go`, `internal/crq/observe.go`, `internal/crq/service.go`	Policy, evidence-bot, co-reviewer, command, and configured-bot helpers now use `Config` receivers.
Configuration threading through observation `internal/crq/observe.go`, `internal/crq/service.go`	Observation, adoption, queue progression, and co-reviewer paths receive and use explicit configuration values.
Feedback evidence and report decisions `internal/crq/feedback.go`	Feedback generation uses config-based policy, evidence-bot, and configured-bot helpers.
Call-site and source validation `internal/crq/codex_replay_test.go`, `internal/crq/service_test.go`, `internal/crq/observe_config_test.go`, `internal/crq/source_test.go`	Tests update changed signatures and verify that `observe` uses its supplied configuration.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

kristofferR/coderabbit-queue#14: Introduces feedback-bot separation used by the config-driven evidence extraction changes.
kristofferR/coderabbit-queue#30: Establishes related observe–decide–apply configuration wiring.
kristofferR/coderabbit-queue#32: Also updates feedback and policy decisions to use configuration-driven flow.

Poem

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the refactor from Service-held reviewer config to Config-based value access.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch refactor/reviewer-config-as-value

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:b0493e6429

ℹ️ 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".

coderabbitai · 2026-07-26T16:34:09Z

✅ Action performed

Review finished.

The parameter reached the top of observe and stopped there. Underneath, the review-shell and reaction filters still asked s.isConfiguredBot, check runs were filtered with s.cfg.coBotEnabled, and reviewCommands searched and adopted commands using s.cfg. That is worse than not taking the parameter at all. A repository that enables Bugbot while the fleet does not would have its check runs fetched and then discarded — so crq triggers a bot already running, or times out waiting for evidence it threw away — and a repo-specific review command would be missed and posted again. isConfiguredBot becomes a Config method, and reviewCommands/adoptableCR take the configuration too. A grep-shaped test keeps observe.go at zero reads of the Service configuration: the bug is a class of site, not one behaviour, and every instance of it compiled and passed.

kristofferR · 2026-07-26T17:04:16Z

@codex review

kristofferR · 2026-07-26T17:06:33Z

@coderabbitai review

chatgpt-codex-connector · 2026-07-26T17:06:35Z

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit:1ed0ed95f9

ℹ️ 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".

coderabbitai · 2026-07-26T17:06:38Z

✅ Action performed

Review finished.

kristofferR · 2026-07-26T17:36:56Z

@coderabbitai review

coderabbitai · 2026-07-26T17:37:02Z

✅ Action performed

Review finished.

kristofferR · 2026-07-26T18:34:54Z

@coderabbitai review

coderabbitai · 2026-07-26T18:34:58Z

✅ Action performed

Review finished.

coderabbitai

Actionable comments posted: 1

🤖 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/observe_config_test.go`:
- Around line 17-21: Update the source-validation test around observe to extract
and inspect only the observe function body, rather than counting s.cfg. across
the entire file. Replace the formatting-sensitive signature substring check with
function-scoped validation, while preserving the requirement that observe uses
its cfg Config parameter and remains independent of Service configuration.

🪄 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: 45e0ce73-0680-4c75-9f18-fb80cb6e63e8

📥 Commits

Reviewing files that changed from the base of the PR and between 081a791 and 1ed0ed9.

📒 Files selected for processing (9)

internal/crq/codex_replay_test.go
internal/crq/feedback.go
internal/crq/observe.go
internal/crq/observe_config_test.go
internal/crq/reviewers.go
internal/crq/service.go
internal/crq/service_test.go
internal/crq/source_test.go
internal/crq/state.go

📜 Review details

🧰 Additional context used

📓 Path-based instructions (3)

internal/crq/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Files:

internal/crq/observe_config_test.go
internal/crq/source_test.go
internal/crq/reviewers.go
internal/crq/service_test.go
internal/crq/codex_replay_test.go
internal/crq/state.go
internal/crq/feedback.go
internal/crq/service.go
internal/crq/observe.go

internal/crq/service.go

📄 CodeRabbit inference engine (AGENTS.md)

Files:

internal/crq/service.go

internal/crq/observe.go

📄 CodeRabbit inference engine (AGENTS.md)

Files:

internal/crq/observe.go

🪛 golangci-lint (2.12.2)

internal/crq/source_test.go

[medium] 19-19: G304: Potential file inclusion via variable

(gosec)

🔇 Additional comments (8)

Counting s.cfg. across the whole file would fail on an unrelated method, or on the words appearing in a comment, so the guard would break for reasons that have nothing to do with what it protects. It reads the function's own body now.

kristofferR · 2026-07-26T19:30:47Z

@codex review

kristofferR · 2026-07-26T19:33:56Z

@coderabbitai review

coderabbitai · 2026-07-26T19:34:01Z

✅ Action performed

Review finished.

chatgpt-codex-connector · 2026-07-26T19:34:54Z

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit:9853acf120

ℹ️ 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".

kristofferR · 2026-07-26T19:52:52Z

@coderabbitai review

coderabbitai · 2026-07-26T19:52:59Z

✅ Action performed

Review finished.

This branch was cut before #51, so it still read the reviewer configuration off the Service: observe with no config argument, and coCommandBodies as a Service method. Both are Config methods now, and the branch stopped compiling the moment main moved under it — the tests here passed only because CI builds the branch, not the merge. Tidy takes a Config through the whole path now: which comments count as a trigger depends on who reviews, so that has to be a value the caller supplies. It reads the fleet configuration today, and per-repo reviewers (#57) can substitute one there in a single line without threading anything new through.

Same as #56: this branch predates #51, so the account-block observation called s.policy(), which is a Config method now. The branch built on its own and broke against main — CI builds the branch, not the merge, so nothing said so.

@codex

* Delete the trigger comments nothing needs any more A PR driven through a dozen rounds collects a dozen "@coderabbitai review" comments and a dozen acknowledgements, which buries the conversation a human came to read. crq now removes its own half of that as rounds progress, and `crq tidy` does it on demand. Deleting a comment crq still reads is how this becomes expensive, so a comment has to clear three guards. It belongs to a round that has PROGRESSED — a live round keeps its command, because that is the comment crq adopts instead of posting another. The bot acted after it, so it was read rather than merely old. And it predates the current head, because adoption only ever considers commands newer than the head commit; delete one of those and the next pump posts a duplicate and buys a second review. Only comments crq posted. Candidates are the command IDs recorded on its own rounds, never anything matching the text — a person's "@coderabbitai review" is their decision to ask, not crq's to erase, and crq posts under the same account so authorship alone cannot tell them apart. Never the bots' own comments. An auto-generated reply can be a rate-limit or skipped-review notice, which crq classifies as evidence and surfaces as a finding, so deleting those would quietly destroy feedback nobody had read yet. A recorded command that is already gone counts as tidied rather than failing: the bot removes some of its own command comments, and a person may have tidied by hand. * Remember the review request each retry replaces A rate-limited round retries, and every retry posts a NEW "@coderabbitai review" — the bot answered the previous one with the rate-limit notice, so it can never be adopted again. crq overwrote CommandID and forgot the old comment, which is why a throttled PR collects a column of identical requests: nothing knew they were there, and tidying could only ever find commands from rounds that had finished. A round now records the commands it supersedes, bounded so a PR that retries all day cannot grow its round without limit, and tidy treats them as spent whichever phase the round is in. They are exempt from the predates-the-head check on purpose: crq's own record that it has posted a successor is stronger evidence than any timestamp. Observed on PR #50 — five heads, nine review requests, three of them for the first head alone. Codex is not affected: it gets one command per head because crq skips the trigger when the bot has already reviewed it. * Only delete the review requests crq wrote itself Adoption was the hole. A round records the command it adopted in exactly the same CommandID as one it posted, so "the command IDs crq recorded on its own rounds" included a person's "@coderabbitai review" — and once the head moved on and the bot had spoken, tidy would delete it. With a maintainer token nothing even objects. A round now records the comments it WROTE, with the reviewer each was addressed to and when it landed, and that list is the only candidate source. Adopting a comment is not writing one. It also replaces the spent-command list: a command missing from the round's current anchors is one the round replaced, which is the same fact without a second field. Four things fall out of carrying the reviewer with the comment: * a co-only round's anchor is the co-reviewer's trigger, not the primary's — unrelated CodeRabbit activity no longer passes for the answer that trigger is still waiting on, and the id stops appearing on the deletion list twice; * a superseded co-reviewer trigger is attributed to the bot that was asked rather than to CodeRabbit; * candidates are filtered against the comments actually on the PR, so a comment tidy already deleted is not DELETEd again on every later pass with the 404 read back as a fresh removal; * state is re-read after the observation and before the deletes, so a round another fleet member started in that window keeps its command. An unreadable head commit now keeps every non-superseded command instead of clearing the guard: the command may well be adoptable again once the read recovers, and deleting it buys a second review. And a round that completes while holding the slot ("cleared") or inside the reviewing sweep is tidied — between them that is most successful rounds, so the automatic cleanup was barely running. * Keep tidy off comments that stopped being trigger comments Four things review found in the tidy pass. A recorded comment ID proves crq wrote that comment, not that it is still the one-line command crq wrote. crq posts under the operator's own account, so anyone can edit it into an explanatory note — and the pass would then delete someone's words as a spent request. The body is now checked against the reviewer's trigger command; anything else, including a command whose config changed since, is kept. A refused delete was only logged, so a caller reading an empty `deleted` could not tell "nothing was spent" from "this token may not delete". The pass still tries every ID and now reports the failures in `.failed[]`. Codex can answer a trigger with nothing but its thumbs-up, and that reaction alone completes the round — leaving no review, event or check for answeredAt to find, so the `@codex review` comment was kept for ever. Tidy observes with no round, so observe() never fetches reactions; they are read here instead, once per candidate nothing else has answered. And the README documented neither `crq tidy` nor `CRQ_TIDY`, so an upgrade started deleting comments with no entry in the documented CLI contract. * Tidy the triggers a force-push and a PR reaction retired Three ways the tidy pass read the PR differently from the code it mirrors. Adoption's cutoff is the head commit date raised to the last force-push, because a force-push can point the PR at a commit object older than the commands made for an earlier head. Tidy compared against the commit date alone, so a rebase onto such a commit kept a trigger no round can ever adopt again — for ever, once the PR merged and the closed-PR guard stopped looking. It now asks for the same cutoff, and TidyInput.HeadAt is renamed AdoptableFrom to say which one it is. Codex answers a command in the PR description by reacting to the PR, and observe() accepts that as the round's completion. Tidy only read reactions on each recorded trigger, so a round that ended that way left no evidence it could see and kept the spent "@codex review" comment. Both of observe's sources count now; the PR's reactions are read at most once per pass, and only when a candidate is still unanswered. And a pump that changed nothing reports "waiting", which tidy treated as progress: every poll of a long-running review bought a second full observation of the PR the pump had just observed. Housekeeping does not get to spend the REST quota the queue runs on. The one "waiting" that does move a round parks it with its commands still live, so it has nothing of its own to remove. * Read the third place a Codex thumbs-up can be A Codex-gated round completes on a +1 left on the trigger, on the PR, or on the command the round fired on — observe() reads all three, and for a round whose primary is CodeRabbit that last one is a different comment from the "@codex review" crq posted beside it. Tidying now reads it too, at most once per candidate nothing cheaper has answered, so a round that ended there stops looking like a trigger nobody read. The README and llms.txt said a comment is removed once it predates the current head; it is the cutoff adoption uses, the head commit raised to the last force-push. * Take the reviewer configuration as a value, like the rest of crq This branch was cut before #51, so it still read the reviewer configuration off the Service: observe with no config argument, and coCommandBodies as a Service method. Both are Config methods now, and the branch stopped compiling the moment main moved under it — the tests here passed only because CI builds the branch, not the merge. Tidy takes a Config through the whole path now: which comments count as a trigger depends on who reviews, so that has to be a value the caller supplies. It reads the fleet configuration today, and per-repo reviewers (#57) can substitute one there in a single line without threading anything new through. * Preserve review evidence when tidying commands * Harden tidy command cleanup * Preserve tidy history and throttle backoff * Retain Codex reaction evidence during tidy * Preserve tombstones after ambiguous deletes * Address tidy review findings --------- Co-authored-by: kristofferR <kristofferR@users.noreply.github.com>

* Let an agent account for a finding GitHub cannot close crq resolve and crq decline both act on a review thread. A review-body finding, a review-skipped notice and an outside-diff remark have none, so neither command can touch them — and drain-first then blocks every future round on a finding that can never drain. The observed end state was a PR reporting that no review was ever requested for its current head, four rounds running: the round could not start because the finding was undrained, and the finding could not drain because nothing could act on it. crq dismiss <repo> <pr> <finding-id>... --reason "<why>" records that the agent judged it. Dismissed findings are withheld from the action, and crq next reports dismissed: N so nothing looks silently dropped. Three choices worth stating. The reason is stored, not just demanded — a dismissal that discards its justification is not auditable. Finding IDs are content-derived rather than GitHub node IDs, so the repo and PR are required to identify one. And the record is scoped to the round for the current head: the same content yields the same ID, so a dismissal that outlived its head would silently swallow the finding when the next reviewer reports it again. Pushing supersedes the round and clears it, which is the rule body findings already follow. Dismissing enqueues the PR when crq is not yet tracking the current head, because that is the deadlock's own signature — no round for the head at all — and a dismissal with nowhere to live would change nothing. * Dismiss only what has no thread, and mean it everywhere Three review findings, all correct. Filtering happened in nextFromState, so a dismissal did nothing for crq feedback or crq loop: convergence and the exit code are computed from the full list, leaving the finding permanently actionable outside crq next. The filter now lives in Feedback, before Converged, so there is one of it. Nothing stopped dismissing a finding that HAS a thread. That would let a round converge with the thread still open, skipping the resolve/decline flow that puts the decision on the PR where the bot can answer it. A threaded finding is now refused by name, and so is an ID that is not a finding at this head at all — a stale copy-paste would otherwise record a dismissal that silences whatever later matches it. And the write is one CAS update again. Enqueueing first meant that a failure between the enqueue and the record left a fire-eligible round behind with nothing dismissed on it, which the autoreview daemon could claim and spend a review on. Dismiss now reads the findings, validates them, then creates or supersedes the round and records the decision in a single write. * Refuse the dismissals that would cost quota or bury a thread Six review findings, all correct, and three of them P1. A round tracking a different head is now refused rather than superseded. If the head moved after Dismiss read the findings, superseding archives the live round and points the queue back at a commit nobody is looking at — and CAS retries cannot protect against a mutation that deliberately overwrites newer state. A round may only be CREATED when this call drains the head. Dismissing one of several open findings used to queue a fire-eligible round that DecideFire cannot hold back, because it never sees findings — so a pump could spend the primary's quota on code the caller is still expected to fix. A finding carried from an older commit is refused. IDs hash the text, not the commit, so recording one against the current head would silently filter the identical finding when the current reviewer reports it. And "no thread ID" turns out not to be the question. When the GraphQL thread query fails, Feedback falls back to REST, which does not return thread IDs at all, so an inline comment with an open thread arrives looking threadless. Only sources that intrinsically cannot have one — review bodies, prompt blocks, skip notices, issue comments — are dismissible now. The CAS write moves to service.go as recordDismissal, which is where AGENTS.md says every write belongs, and it honours DryRun. * Give crq its own place on disk for a repository Everything crq does with git assumes it was RUN inside the checkout it cares about: the local-work probe shells out with no directory, target inference reads the current branch. That is right for a command an agent types from its working copy, and useless for the daemon, which has no checkout of any repository it reviews — the reason dispatch has nothing to stand on. Workspace separates the two: a bare mirror per repository, fetched rather than re-cloned, and a throwaway detached worktree per head. Detached on purpose — a place to inspect and build, not a branch to commit to by accident. A worktree left behind by a killed process is replaced rather than reused, since the head it holds is stale anyway. Credentials stay out of it. The remote is the ordinary https URL, so the host's existing git credential helper supplies the token and crq never holds a secret it would then have to avoid logging. One runner now executes every git command, taking the directory to run in, and it folds stderr into the error — "exit status 128" alone has never told anybody what went wrong. localWork takes that directory too, through Config.WorkDir, so a caller working in a worktree it made can say so without the Service being copied. The tests build a real repository and clone it, because the thing under test is whether the git invocations are right. * Act on a transition instead of logging it The loop had a hole at the end: crq could say a PR needed fixing, and nothing turned that into work. A supervisor script polled, wrote the answer to a file, and logged the change — and then depended on somebody reading the log. Tonight that somebody did not, and eight findings sat for an hour. crq watch drives every open PR through the same next oracle an agent uses and emits one JSON line per PR per pass. With --dispatch, a PR whose action is fix gets a session started for it, in a worktree crq checked out at the head the findings are about, with the findings on disk and the target in the environment. This keeps the queue's non-goal intact. crq still does not decide which findings are real: it starts the session and says which PR to look at, and the session judges. That is why dispatch is a separate command over the same oracle rather than something the pump does, and why it is off unless asked for — watching is an observation, dispatching writes code. Two bounds make it safe to leave running. Every dispatch is claimed under compare-and-swap with a heartbeat, so two watchers cannot both work one PR and a session that dies frees its round after the TTL rather than holding it forever. And attempts are counted per head, so a fix that keeps not working stops instead of spending a review round each time; a new head clears the count, because reaching one means the attempt achieved something. The command runs directly, not through a shell, so nothing expands that the operator did not write. * Keep one checkout from destroying another, and private source private Six review findings on the workspace, two of them P1. The mirror tree was created with the umask's 0755, so on a shared host git wrote a private repository's objects and source world-readable under ~/.cache. The root is 0700 now, enforced rather than assumed. Clones were unauthenticated in the documented token-only setup: git does not read GITHUB_TOKEN or GH_TOKEN by itself, so a daemon with no credential helper would have failed at its first private checkout — dispatch's first real act. A credential helper is injected when crq has a token, and the token travels in the environment, never in argv, so a process listing, a log line and this package's own error strings carry the helper snippet and not the secret. Three ways a checkout could destroy another. Two workers first cloning one repository both passed the missing-HEAD check and cloned into the same directory; the clone now lands in a staging directory and is moved into place, and losing that race is fine because the winner's mirror is just as good. "a-b/c" and "a/b-c" joined with a dash are the same path, so one repository's cleanup deleted the other's live worktree; owner and name stay separate components. And a deferred Remove on a stale handle deleted the checkout that replaced it — each checkout now owns a generation directory and removes only its own. CRQ_WORKSPACE is read through Config, so a value in ~/.config/crq/env is actually used instead of being silently ignored by the daemon. * Make a repeated dismissal succeed, and a partial one refuse Three more findings, one P1. The eligibility guard asked whether a round object existed, when what matters is whether a FIRE-ELIGIBLE one would. A queued round is exactly as dangerous as a newly created one: Pump can hand it to DecideFire, which sees no findings and cannot enforce drain-first, so dismissing one of several open findings could still buy a review of code the caller is still fixing. Repeating a dismissal failed on its own earlier success: Feedback filters the dismissed ID out, so validating against the current findings alone rejected it as unknown. The command is documented as idempotent and an interrupted agent repeating itself is the ordinary case, so an ID this round already dismissed is accepted and reported as such. And the filter now checks the SOURCE, not just the ID. Finding IDs hash the text, not where it came from, so a dismissed body finding later delivered as an inline comment through the REST fallback hashes the same — and an ID-only filter would hide a review thread that is open. * Authenticate the way the API client already does Four findings, one P1. Reading only GITHUB_TOKEN and GH_TOKEN meant the documented `gh auth login` setup produced unauthenticated clones: API calls worked, and every private checkout failed at dispatch's first real act. git now gets the same token the API client resolves, `gh auth token` included. A relative workspace root put the worktree somewhere other than where the returned path said. `git worktree add` runs inside the mirror, so the relative directory landed under the mirror while Checkout.Dir pointed at a path that did not exist. Roots are absolute now. Clearing a PR's directory before making a new generation force-removed a checkout another worker might be building in. Old generations are pruned by age instead, which collects what a killed process left without touching a live session. And two workers fetching one mirror race on git's ref locks, so the loser reported "cannot lock ref" although the winner had just made the mirror current. That retries briefly and then accepts the mirror as it stands, rather than failing a dispatch over a fetch somebody else finished. * Supersede the stale round instead of refusing the dismissal Three findings, one P1 in the fix itself. After a push the stored round is still on the PREVIOUS head, because `crq next` returns on a current-head finding before it enqueues. Treating that as a concurrent move rejected every dismissal and left the new head in the exact drain-first deadlock this command exists to end. The two cases are told apart by when the round was enqueued: after the findings were read means somebody moved the PR forward and the decision is stale; otherwise it is the ordinary post-push round, and it is superseded. A finding another agent dismissed concurrently now counts as handled when deciding whether other work is still open, instead of making this call refuse over a finding that is already accounted for. And the docs no longer send an agent to a command that will refuse it: a `review_comment` finding lost its thread ID to the REST fallback, so it is neither resolvable nor dismissible until crq can read review threads again. * Stop the dispatcher losing work, quota, and its own arguments Ten findings, six P1. `crq watch --dispatch -- <cmd>` never worked: FlagSet.Parse consumes the terminator, so looking for it afterwards found nothing and the fix command was read as a list of repositories. argv is split before parsing. Five ways dispatch could do harm. It ignored DryRun, so the mode that promises crq writes nothing claimed shared state and ran a code-writing command. It wrote the findings at the worktree root, where a session following the documented `git add -A` push would commit crq's review payload into the PR — they go outside the checkout now. It deleted the worktree after a successful session, discarding fixes that were made but not pushed; a worktree with uncommitted work is kept. It ignored the fleet skip marker before calling Next, which enqueues and can fire — the marker exists to protect the shared quota, so it is honoured first. And a lost heartbeat left the session running while another watcher started a second one on the same worktree; losing the claim now stops the session. Attempts were counted at the claim, so a failed clone ate the per-head budget and permanently skipped the PR after three tries. The attempt is given back when nothing ran. Two smaller: a throttle now sleeps for the reset the API named instead of the ordinary interval, which was hammering an exhausted quota, and a one-shot pass reports the PRs it could not read instead of exiting 0 — cron reporting a clean scan of a broken one is worse than a failure. An emit failure stops the watcher, because a closed pipe means nothing is observing something that fires reviews and starts sessions. * Stop the mirror deleting the branches sessions create Four findings, one P1 that would have destroyed a fix session's work. The mirror was a --mirror clone, whose refspec is +refs/*:refs/*, so the next `fetch --prune` reached into refs/heads and deleted any branch a session had created in its worktree — the documented way for a session to make changes. It is a bare clone now, fetching into refs/remotes/origin/*, which leaves refs/heads to the sessions. Pruning read the checkout directory's own timestamp, which editing files inside does not update: a session busy for twelve hours read as abandoned and had its worktree force-removed. It measures the newest file under the checkout instead. Persistent ref-lock contention on a shared mirror returned an error even though the mirror was current, failing a dispatch over another worker's success — the exact case concurrent dispatch has to survive. And the worktree add now goes through the credential-carrying runner, so a checkout filter that fetches from a private repository is authenticated like every other git call. * Judge a cooling round by whether it will fire, not whether it can now Three findings, one P1 in the guard I added last round. FireEligible answers "right now", which is the wrong question when deciding whether a partial dismissal would leave a round able to buy a review. A round cooling in awaiting_retry is not eligible this second and becomes eligible the moment RetryAt passes, so letting the dismissal through on that technicality just deferred the hazard to the cooldown expiring. The guard asks whether the round will ever fire again. State alone could not tell "no round for this head" from "the head moved and nothing has enqueued it yet", so a push landing between Feedback and the write could record the dismissal against the wrong commit. The head is re-read once before recording. And the README now carries the same caveat as the skill: a review_comment finding lost its thread ID to the REST fallback and still has an open thread, so it is neither resolvable nor dismissible until crq can read threads again. * Dismiss the same repository it validated * Migrate a mirror's refspec instead of only setting it at clone time Applying the refspec only when cloning left every mirror made by an earlier crq still fetching +refs/*:refs/*. A fix session that created a branch in its worktree then wedged the WHOLE repository: git refuses to fetch into a branch checked out somewhere, so every later checkout of every PR failed with "refusing to fetch into branch ... checked out at". Observed live — one session's branch stopped the drain from dispatching anything for hours, while PRs sat with findings nobody was looking at. The refspec is now enforced on every Mirror call, which migrates the mirrors that already exist. The test reproduces the original failure: an old refspec, a branch created in a worktree, and a fetch for a different PR that has to keep working. * Merge the mirror refspec migration * Say it out loud when no fix session can start The dispatcher failed for hours and looked like success from outside: the watcher ran, the queue moved, PRs reported findings — and every session died on a wedged git mirror, in a log line nobody was reading. The only thing that noticed was a person eventually asking why a PR was untouched. A pass that attempts a dispatch now records whether one actually STARTED. Three failed passes in a row is a dispatcher that is not working, and crq says so where it is seen: a 🚨 line on the dashboard naming the host and the error, and the status line saying "dispatch failing" above every other state — a queue that looks busy while nothing can start is worse news than anything the queue itself reports. It is deliberately not Warn. Warn is cleared by the next successful fire, so unrelated progress would wipe it, which is how this stayed invisible. A claim another watcher holds does not count as a failure, and one started session clears the alarm. * Run fix sessions beside each other, not inside the loop A session ran inline in the pass, so one twenty-minute fix meant twenty minutes in which no other PR was even looked at. The drain went quiet for six minutes at a time with six PRs waiting and one session working. Sessions now run in a bounded pool off the pass, up to CRQ_DISPATCH_CONCURRENCY (default 3). When every slot is busy the PR is left for the next pass rather than queued behind a session — waiting here would recreate the problem the pool exists to solve. The DECISIONS stay serial, deliberately. `Next` is what enqueues and fires, so deciding one PR at a time is what keeps the account-metered review in a single queue; only the sessions overlap, and they spend no CodeRabbit quota. Concurrent pushes still land in that one queue and the fire slot serializes them as before. Dispatch health moves into the session, since the pass no longer knows the outcome by the time it ends, and a --once run waits for its sessions so it cannot return while they are still writing. * Give a fix session a log, and stop killing it when it succeeds Two defects that together explain why sessions committed code and never resolved anything. A session's push moves the head, so crq supersedes the round and the fresh one carries no dispatch claim. The heartbeat read that as "another watcher took this round" and killed the session — between the push and the resolve, every single time it worked. Losing a claim and having it STOLEN are now different answers: only a claim somebody else actively holds stops a session, and a superseded round just stops the heartbeat. And the session's output went to nowhere at all, so a failed one left nothing to read but the absence of a result. Every session now writes to $CRQ_WORKSPACE/logs/<owner>/<name>/<pr>-<head>-<time>.log, the path is logged before it starts so it is findable while it runs, the last five per PR are kept, and a failed session keeps its worktree so there is state to look at. The unit, wrapper and prompt move into examples/dispatch/ so they are versioned rather than living on one machine. The prompt's two hard-won rules are documented where the next person will need them: stay detached and push by ref, because the worktrees share one mirror and a branch checked out in one makes git refuse to fetch for every PR. * Make the unattended drain one command Setting it up meant copying three files, remembering `loginctl enable-linger`, and getting the environment right — and setup people get wrong is setup that silently does nothing, which is the failure this whole feature exists to prevent. `crq drain install` writes the prompt, a wrapper and this platform's service definition (a systemd user unit, or a launchd agent on macOS), turns on what that platform needs to survive a logout, and starts it. `--dry-run` prints every path and command first, so it can be reviewed before anything is touched. A missing fix agent fails here rather than at the first dispatch, where it would look installed and fix nothing. The prompt is embedded, so the bytes crq installs are the bytes in the repository and a rule learned the hard way cannot drift between them. The copies under examples/ are gone for the same reason; the README there now explains the two rules and where to look when it misbehaves. * Create the directory the service logs into systemd refuses to start a unit whose StandardOutput path cannot be opened — 209/STDOUT — so the generated unit pointed at a log directory nothing created and the drain never ran. Install makes it first. Caught by installing with the command instead of by hand, which is the point of having the command. * Make a session write its log while it works claude -p only emits at the END, so the log stayed zero bytes for the whole run and a session that hung or died mid-way left nothing at all — the same "output went nowhere" problem with a file to show for it. The generated wrapper asks for streamed output, so the log fills as the session goes. * Rotate the pass so the tail is not starved of dispatch slots A watch pass walked PRs in the same order every time, so with three dispatch slots and four PRs needing fixes the same three took the slots and the fourth was told "at dispatch capacity" every pass, forever. #54 sat five hours that way while its findings grew from 15 to 25 — reviews kept landing and nothing was ever dispatched to answer them. Candidates are gathered first and the starting point advances each pass, so every PR reaches the front. This is the fix the quota-free rescue scan already needed for the same reason: a fixed start means whoever is behind the front is behind it permanently. * Stop queueing the work that spends no quota Dispatch had a three-slot queue in front of it, and the queue exists for exactly one thing: the account-metered review. Fixing findings spends none of that allowance, so making a PR wait for a dispatch slot queued work with no reason to wait — the same mistake crq already corrected for co-only rounds, which bypass the slot and the quota gate outright. It cost what that always costs: #54 was told "at dispatch capacity" every pass for five hours while its findings grew from 15 to 25. Rotating the order made that fairer, which is a patch on a queue that should not have been there. There is no cap by default now. A PR whose findings are ready gets a session. CRQ_DISPATCH_CONCURRENCY still sets one for a machine that cannot take the load, and hitting it says so in those terms — a resource valve, named as one, not a routine "capacity" message. * Stop the drain from losing what it was supposed to protect Review of the unattended drain found several ways it fails quietly, and they share a shape: something goes wrong and the outcome still reads as success. Work: a session that COMMITS its fixes and does not push them leaves a clean working tree, and the cleanup deleted that worktree — the only copy of the fix. Cleanup now asks the remote whether the commit arrived, and keeps the checkout for anything it cannot establish, including a status probe that errored. A lost fix is not recoverable; a kept worktree is pruned by age. Attempts: a command that never reached a process — a mistyped agent, a file without the execute bit — spent one of the three attempts for that head. Three passes of that and the PR was permanently undispatchable even after the configuration was corrected. Only a process that actually ran spends an attempt, which is what releaseDispatch already said it did. Heads crq never queued: `Next` returns fix before enqueueing when findings are already in hand, so a PR reviewed outside the queue had no round for the claim to live on and was refused forever. The claim adopts the head as reviewed — which it demonstrably is — without making it fire-eligible, so no review is bought. The heartbeat cancelled a session for a round tracking ANOTHER head, which is what its own push produces: it killed successful sessions between pushing and resolving. It now stops beating instead, and only a live claim from somebody else counts as a theft. It also starts before the checkout, since a first clone can outlast DispatchTTL. Also: CRQ_DISPATCH_CONCURRENCY was read after the pool was built, so an operator's cap was ignored unless --concurrency repeated it; a throttled --once exited 0 after checking part of the fleet or none of it; a dry run wrote dispatch health; the gate repo's calibration PR was watched like work; dispatched=true was emitted before the round was claimed, so a competing watcher's PR looked handled; logs settled one over their bound; a quoted CRQ_DISPATCH_CMD argument was split apart; a --agent that cannot be resolved and a service that fails to start were both reported as a working install; the generated unit inherits a PATH where git, gh and crq are findable; and a fork PR's head is fetched from refs/pull/<n>/head, which is the only ref that reaches it from the base mirror. * Let a fix session push the work it just did The push every dispatched session is told to make — `git push origin HEAD:refs/heads/<branch>` from its detached checkout — cannot work. `clone --bare` sets remote.origin.mirror, and git then refuses a push with a refspec outright: "--mirror can't be combined with refspecs". A session could fix the findings, commit them, and never land them; this session hit it trying to push these fixes. The mirror flag is turned off wherever the fetch refspec is set, on every call rather than only at clone time, so mirrors already on disk are healed too — the same reason that rule is re-applied each time. * Learn the account block from any notice, not just the round that asked crq kept posting @coderabbitai review while the account was rate-limited — #54 got one at 01:03:15 and another at 01:07:30, four minutes apart, inside the window the first one's reply had already announced. The gate was never the problem. The block was only ever derived inside Progress, which needs the round that fired the command to still exist and to have fired before the notice arrived. Neither survives a fix session's push: the head moves, the round is superseded, and the rate-limit reply it was waiting for is archived unread. crq then genuinely believed the account was free, so it asked again — and the more sessions pushed, the more often it forgot. The allowance is an account-wide fact. Any current notice from the primary now records it, whatever round it answered, before the fire decision is made. AcceptAccountBlock still decides whether it replaces the standing window and still never shortens one. Two things it must not do, both caught by the replay suite. An edit is not new evidence: CodeRabbit rewrites its notice in place as the window counts down, and treating each edit as a fresh block renews it forever from a message already accounted for. And a notice whose own window has passed only earns a fallback block if crq has not already seen it, or an hour-old message would start a new window every time it was read. * Stop the calibration probe shortening a block it cannot see Calibration replaces the whole account quota with its reading, which made it the one writer that could SHORTEN a standing block — every other one goes through AcceptAccountBlock, which never does. That was survivable while a block only came from the round that fired the command. It is not now: a PR's own rate-limit notice records the window CodeRabbit stated, and a probe whose reply carries no parseable reset would erase it. Pump then fires inside that window, which is the duplicate-review behaviour the whole system exists to prevent. A probe reports no block two ways — still awaiting a reply, or a reply with nothing parseable in it — and neither is evidence the account is clear. Only the first was handled. A still-active standing block now survives both, while a LONGER window from the probe still wins, because that is new information. The calibration PR itself (kristofferR/crq-state#1) is unchanged and healthy: 7 comments, pruning active, out of every watch scope. * Leave refs/heads to the sessions, and stop swallowing a failed fetch Review of the workspace found six ways it could hand a session a repository it could not work in. A bare clone copies the remote's branch heads straight into refs/heads, and the refspec set afterwards only governs later fetches — so the namespace this PR reserves for sessions arrived already occupied, with a branch a session cannot create and a commit frozen at clone time. Init and fetch instead, which leaves it empty. A mirror an older crq made with --mirror also carries remote.origin.mirror=true; the refspec migration did not clear it, and a plain push from a worktree would have mirrored the whole local namespace. Existing mirrors keep their refs/heads: a branch there may be a session's work, which is exactly what must not be deleted. The retry loop tested for the mirror's HEAD after a failed fetch, and HEAD is there because the mirror is there — so an expired token or an unreachable remote came back as a current mirror with stale refs, and reached the caller later as an unreadable commit instead of the error that explains it. Only ref-lock contention is somebody else's success; everything else is propagated. A PR opened from a fork has its head on no branch of the base repository, so the refspec never brought it down. Fetch refs/pull/<pr>/head when the commit is missing, best effort, since the checkout that follows is the real check. Checkout.Git ran without the workspace's credentials, so a daemon holding only GITHUB_TOKEN could clone but a session's push could not authenticate. Stale generations were collected only under the PR being checked out, so one left by a killed process outlived the PR that would have swept it; pruning now sweeps the repository. Also: `--` before `worktree add`'s positional arguments, and removeWorktree is a plain function rather than a method needing a throwaway Workspace to call it. * Refuse the partial dismissal that would queue the new head A round left on the previous head can be past firing, so canStillFire said no and the guard let a partial dismissal through — and the supersede below then replaced it with a fresh queued round for the new head, which is exactly the fire-eligible round the guard exists to prevent while findings are open. Tell a stale round from a live one by Seq rather than by comparing its EnqueuedAt against this host's clock: the fleet's hosts do not agree about the time, and a worker whose clock lagged could supersede a newer round back to a commit nobody is looking at. A replay of a dismissal that already succeeded writes nothing, so it now returns before the guard instead of being refused whenever some other finding is still open — the command is documented as idempotent. The dry run runs the same mutation against a throwaway copy of the state, so it reports the ids it would record and the refusals it would hit instead of a blanket success it cannot drift from. Also clear the dismissal count when `crq next` finds the head moved (it is head-scoped), and name issue-comment findings in both threadless-finding lists, which dismissibleSources has covered all along. * Pin the replayed dismissal the guard used to refuse The idempotence the short-circuit restores had no test that could fail: the existing replay runs against a completed round, which the guard lets through anyway. This one drives the case that broke — a dismissal that ended the deadlock leaves a QUEUED round for the new head, a later review reports something else, and the agent repeats the dismissal it does not remember finishing — and asserts the replay is both accepted and a no-op. * Give the drain a head it can dispatch and a service that can run Seven fixes to the unattended drain, all of the same shape: something the watcher decided correctly was then thrown away. A stale round refused the new head forever. `Next` reports fix without enqueueing when the findings are already in hand, so nothing supersedes the round for the head that moved on — and every pass answered "no round for this head" and counted it as the dispatcher failing. The claim now supersedes it, unless a session is fixing an earlier head right now: its own push is what moved the head, and it is entitled to finish. Adopting a head no longer manufactures review evidence for it. Marking a head reviewed to hold the claim is right when the findings are about that head, and wrong for feedback carried from an older commit: the completed round dedups the review away while the caller waits for one that can no longer be requested. The attempt bound is crq obeying its own configuration, not fix sessions failing to start. Counted as drain health, a correctly bounded head raised "fix sessions are not starting" after three passes, and every pass after that. `--concurrency 0` is the documented way to turn off a configured cap for one run, and an int could not tell that from the flag not being passed. A rate-limit notice is spent when crq has accounted for THAT NOTICE. Account.CheckedAt is also advanced by a calibration probe still awaiting its reply, or one whose reply had nothing parseable in it — neither is evidence the account is clear, and both discarded a notice crq had never seen, after which the round fired inside the block just reported. The fix prompt pushed to `origin`, which for a fork PR is the base repository: the commit lands on a same-named branch there and the PR never sees it. It also demanded Go validation of every repository the account watches; it now asks for the target repository's own checks. And the service inherits none of the installing shell. The unit now names the config file the install read and the queue's identity, config files can hold the GitHub credential, and an install that would leave the watcher unable to authenticate refuses instead of reporting Started. * Answer credentials for GitHub only, and stop pruning live checkouts Three things the review found in the workspace. The injected credential helper answered every request, whatever the protocol and host. A git command can be led to a URL that repository CONTENT chose — a submodule, an LFS endpoint — so a pull request could have pointed one at its own server and been handed the account's token. It now reads the request off stdin and stays quiet unless it is https://github.com. `git config remote.origin.fetch` ran on every Mirror call, and git serializes config writes through config.lock: two dispatches of one repository collided with "could not lock config file" before reaching the fetch retry that concurrency was supposed to survive. Reading the value first takes the write out of every call that has nothing to migrate, and the migration itself now retries. Pruning by age alone deleted a checkout whose process was still alive: a session holding uncommitted fixes while it waits on a reviewer touches no file for hours. A live handle now refreshes its own directory timestamp, so ageing out means the owner is gone rather than quiet. * Let the fixes land instead of holding them for a queued round A round is no longer created only by an enqueue: `crq dismiss` creates one so the decision has somewhere to live. `crq next` read any round at all as "a review is running for this head", so the documented fix flow — fix locally, dismiss the threadless finding, call again — was answered `hold`, telling the caller to sit on the very fixes it had just made until a review of the code they replace finished. That call then enqueued and pumped, so it could buy that review itself. A queued round has asked for nothing: no command posted, no quota spent, and a push supersedes it for free. Only a round that actually got as far as reserving the slot is a review worth holding a head for. Also count the dismissals the round recorded rather than the current findings that still match one. A finding ID hashes the text, so a bot editing or deleting the comment a dismissal was made against left nothing to match and `dismissed` fell back to zero — making a finding deliberately set aside read exactly like one that was never reported. * Pin the other side of "a review worth holding for" reviewRequested draws a line between queued and reserved, and only the queued side is asserted. Pin the reserved one too, so a later widening of the predicate releases a head with the command already about to be posted and fails here instead of on a PR. * Finish the migration of a mirror an older crq made Rewriting the fetch refspec does not move the branches `git clone --mirror` already copied into refs/heads. Those copies keep the names refs/heads reserves for the sessions — `checkout -b feature` fails with "a branch named 'feature' already exists" — frozen at the commit that clone saw. The fetch has just written each one's current value under refs/remotes/origin, so drop the copy: only for names origin actually has, and never one a worktree has checked out, because `update-ref -d` deletes a checked-out branch, work and all, in silence. Three more holes in the same migration: remote.origin.mirror was unset with the error discarded, so a lock held by a concurrent write left the flag in place and Mirror handed back a repository whose later plain push still mirrored every local ref — the hazard the unset exists to remove. Retry it like every other config write and verify the absence. A key holding several values answers `config --get` with the last of them and refuses a single-value write outright, so a mirror carrying a second remote.origin.fetch read as already current, or failed every Mirror call for that repository for good. Read with --get-all, write with --replace-all. A ref lock left behind by a killed git never clears and reads exactly like a live race for as long as it sits there, so retrying past it returned refs known to be stale as current ones — which the caller met later as an unreadable commit that named neither cause. Report the fetch that did not happen. * Keep a session's branch and collect a checkout stamped ahead dropFetchedHeads told a clone's leftover from a session's own branch by whether a worktree had it checked out, so a session that detached HEAD to look at another commit lost the branch it had committed to — its ref was the only thing keeping those commits reachable. Delete only a branch whose commits origin already has, which is what makes the copy redundant in the first place. newestModTime took a file's timestamp at face value, and one stamped in the future — an extracted artifact, a clock corrected backwards — made time.Since negative and so forever under staleWorkAge. Ignore those rather than clamp them to now, which reads as 'touched moments ago' just the same. * Stop the drain wasting the work it is in the middle of Twelve review findings on the unattended drain, mostly one shape: crq knew something and then acted as though it did not. A round somebody is actively fixing stayed in the fire queue. The claim is the only record of a session — the round stays queued throughout — so the daemon could spend the account's metered review on a head the session was about to replace. It now leaves the queue while claimed, rather than being refused at the front of it, so every other PR keeps moving. And the claim survives the session's own push. Enqueue runs before the dispatch guard, and enqueueing at a moved head supersedes: the round the session holds is archived and a fresh, claim-less one takes its place, so the guard saw no session at all and started a second one against work the first was still landing. The archived claim is consulted, and released when that session finishes rather than left to time out. A rate-limit notice is now recorded wherever it is seen. Pump only ever looks at the PR it is about to fire; a notice on a superseded round — or on any PR that is not next — was read by Feedback and discarded, and the next fire went out inside the window the bot had just stated. Recording it also syncs the dashboard, which otherwise advertised a free account for the whole block. The calibration probe may not shorten a standing block, but a reply that says reviews are left is not "no block observed": it is the account reporting itself available, and keeping the old window over it left state saying both at once. The drain's own install: CRQ_CONFIG is resolved to an absolute path before it goes into a unit that starts elsewhere, the unit carries CRQ_STATE_REF so the service reads the fleet's queue rather than the default, and a token-only credential now reaches the session's `git push` — the mirror records the credential helper (the snippet, never the secret) and dispatch supplies the token through the environment. One repository being renamed or unreadable no longer aborts the pass: the rest of the fleet is scanned, --once still reports what it could not check. A fork push is confirmed through refs/pull/<n>/head, the one ref that sees it from the base repository's mirror, so a landed fix stops reading as unpushed work. Stale worktrees are swept across the whole repository, not only the PR being checked out — a closed PR's leftovers were visited by nothing. And mirror configuration is read before it is written, so two concurrent checkouts of one repository stop racing on config.lock. * Let the drain run an agent other than claude The wrapper hardcoded claude's flags — -p, --permission-mode, --output-format — so --agent codex produced a command full of flags codex does not have. The agent was configurable in name only. crq now knows how to CALL each agent it supports and nothing about which model that agent should use: claude gets its prompt and streamed output, codex gets `exec --skip-git-repo-check` with the prompt as its positional argument, and both are told to act without a human approving each step. The model and reasoning effort belong in the agent's own configuration or in --agent-args. A review queue choosing your model would be the wrong thing owning that decision. An agent crq has no invocation for is refused rather than run with a guess, unless --agent-args says how to call it. --dry-run now prints the exact invocation, which is the part worth reading before it runs unattended. * Make a reinstall replace the running drain "systemctl enable --now" does nothing to a unit that is already active, so installing again with a different agent or prompt rewrote the files and left the old process running. The install reported success and changed nothing — which is how a drain kept dispatching claude after being reinstalled for codex. It restarts the service now. Caught by reinstalling with the command rather than by hand, again. * Read the policy from the configuration, not the Service Same as #56: this branch predates #51, so the account-block observation called s.policy(), which is a Config method now. The branch built on its own and broke against main — CI builds the branch, not the merge, so nothing said so. * Harden and isolate repository workspaces * Fix unattended review drain edge cases * Pin one fix session per pull request, at three levels Observed live: three sessions ran on #54 at the SAME head, minutes apart, each pushing and each drawing a fresh review round — its findings climbed from 19 to 27 while they raced. State afterwards read `dispatch=NONE`, so nothing had refused the later passes. These pin the invariant the claim exists for: a second dispatch for a head somebody is already fixing is refused, the claim is actually persisted on the stored round, and it survives both a full Next call and another daemon enqueueing and pumping the same PR on the shared state ref. All three PASS against the current code, which means the mechanism behind the live failure is still unidentified — it is not a plain missing refusal, not Next or Pump dropping the claim, and not the two-writer interaction I assumed. The tests are worth having anyway: they hold the invariant while the search continues, and the last two changes to this code were made without one. * Make a dispatch claim say what it saw Three fix sessions once ran on one pull request at one head, and the round showed no claim at all afterwards. Three tests now pin the refusal at three levels — a second claim, a claim across a real Next, a claim across a second daemon driving the PR — and all three pass, so the mechanism is not a missing refusal and not the two-writer interaction it looked like. What is left is that the log said nothing. Each claim now records the round it read: its Seq, head, phase, whether the claim reads as held, and the claim's own token, host, attempts and heartbeat, plus whether an archived round still holds one. Seq is the discriminator — two grants naming one Seq mean a claim was lost, two naming different ones mean the round was replaced underneath them, and the line says which. Also fixes a doubled doc comment on beatDispatch, and loosens the mirror-mode assertion to accept the key being unset as well as false, which is what the repo-local workspace branch leaves behind when the two land together. * Leave the mirror usable by the caller that works in it A worktree is made for somebody else to work in, and that somebody runs a plain `git push`. Every git command in this package injects the credential helper with -c, which lasts exactly as long as one command, and git reads no GITHUB_TOKEN of its own — so on a host holding only a token, a caller could do all of its work in a checkout and fail at the last step of every one of them. Merging this as it stood would have taken that fix away from the drain again. So the mirror carries the helper SNIPPET in its own config, written on the clone path as well as the migration one: another worker picks a mirror up as soon as it is renamed into place, and would otherwise find one with nothing for its own commands to use. The secret is still not on disk — what is persisted reads TokenEnv from the environment, so a mirror somebody finds hands out nothing. TokenEnv is exported for the same reason: a caller running git in a checkout has to set it, which makes the name part of the contract rather than a detail. The read uses gitDir rather than w.git deliberately: w.git injects a helper of its own, so the check would answer with that injected value and conclude the mirror was configured when its config was empty. * Fix unattended drain review failures * Preserve detached session branches during mirror migration * Name every other crq that writes the same state Three fix sessions ran on one pull request at one head, twice, and the round showed no claim at all in between. The claim was written; something erased it. The state ref's own history says what. One revision dropped `dispatch`, `dispatch_hold_*`, `writers` and `drain`, and `cobots` from all 57 rounds that had it, while keeping the legacy Codex twins — the signature of a binary that predates both those fields and the version tolerance meant to carry them. It was a crq built two days earlier, still sitting in GOPATH/bin, and the state ref is account-wide, so one stale copy on one host erases the fleet's bookkeeping. Nothing in this build can prevent that: the binary that does the damage is by construction older than any mechanism added to stop it, and version tolerance only helps for a binary that has it. What is possible is to say so, so `crq doctor` now lists every other crq the host can run and flags the ones that are not this build. Compared by content, because every build reports "2.0.0-dev" and the version string cannot tell two of them apart. GOPATH/bin is scanned whether or not it is on PATH: the binary that caused this was not on the daemon's PATH and ran anyway. * Retry interrupted legacy head cleanup * Do not run a fix session on somebody else's branch A dispatch checks the head out and runs an agent over it with approvals bypassed, holding a token that can write to the repository. On a pull request from a fork that code belongs to a stranger: a build script, a test, or an instruction file in the branch runs with the account's credentials on the host. For a fleet driving its own branches that is the point; for a project accepting contributions it is a way to hand the account away by installing a daemon. So a fork is skipped unless CRQ_DISPATCH_FORKS says otherwise, and the event says why rather than going quiet. Reviewing a fork is untouched — reading a pull request runs nothing. An unreadable head repository counts as a fork, not as ours. A deleted fork answers with an empty name, and defaulting the missing case to "same repository" would grant exactly the untrusted one the permission it lacks. This is a trust boundary, not a sandbox, and the documentation says so: crq has no sandbox to offer. The fake GitHub now names the head repository the way GitHub does, or every test pull request would read as an unreadable fork. * Decide the heartbeat's verdict inside the attempt that reached it taken and gone were declared per tick and written inside the CAS closure, which Update re-runs on a conflict. A verdict left behind by an attempt that lost would then be read as this attempt's, and the two outcomes are a session killed for a takeover that did not happen and a heartbeat that stops while its claim is live. Neither is reachable today: the paths that set them return ErrNoChange, and Update returns on that without retrying. But the reasoning needed to see that lives in another package, and the fix is one line. Reset them where the decision is made. * Address review feedback on dismissal flow * Read the skip marker the same way the watcher's oracle does watchPass tested for the marker with strings.Contains, so a pull request that merely documents it — the string in backticks, in the sentence explaining what it does — was skipped before the oracle was consulted. No round, no event, no log line: the pull request simply never appears in a pass. Config.SkipsReview arrives with the hold branch, which owns the marker's semantics; this is the same file, byte for byte, so the two merge without a conflict and the watcher cannot drift from fleet auto-review's reading of it. * Protect active workspace checkouts * Fix unattended dispatch edge cases * Fix unattended dispatch review handling * Harden shared mirror refreshes * Address review drain edge cases * Isolate shared workspace Git state * Fix detached review dispatch handling * Harden shared workspace isolation * Harden unattended review dispatch * Fix drain review edge cases * Answer the five findings the drain's own review raised Each verified against the branch first; all five were real. A fenced block opens only at the start of a line. nextFence searched the whole body, so a mid-line ``` run inside prose opened a block no closer position could match — the fence read as unclosed, everything after it was discarded, and a skip marker further down went unseen. crq then fired a review the author had opted out of. Mid-line runs belong to the inline-span pass, which runs next. `crq drain install --dry-run` is documented as a preview, and could not be run by anyone who had not authenticated yet: run() builds the GitHub client before the command switch. The plan reads no GitHub state, so it is now computed by DrainPlan from configuration alone and answered before that client exists. The flags are parsed by one function shared with the real install, so the two cannot disagree about what was asked for. The generated unit carries CRQ_CALIBRATE_TTL and CRQ_RL_FALLBACK. The service does not inherit the installing shell, so a deliberately longer fallback set there was folded into the config, written into no unit, and silently replaced by the default — the watcher then retried a review command earlier than configured for every block whose window it could not parse. Sessions run under a context Watch can cancel, cancelled before pool.wait(). Returning an error used to fall straight into that wait with the sessions still running: agents kept writing code with nobody observing the watcher, and the error the caller needed could not surface until the last session exited. The fix prompt no longer tells a session to push as soon as its tests pass. It resolves or declines first, then asks `crq next --wait` and pushes only on `action: push` — moving the head while a required reviewer is still reading that commit restarts or strands the review and spends another metered round, which is the hold the oracle exists to enforce. * Take the hold branch's Markdown scanner for the skip marker Both branches carry this file so the watcher and fleet auto-review cannot drift apart in how they read the marker, and the two copies had drifted anyway: the hold branch grew a line-based scanner that understands block quotes, list containers, lazy paragraph continuation and delimiter-run widths, while this one kept the first sketch — which the review caught reading a mid-line ``` run as a fence opener. So take that one, byte for byte, tests included. It passes the fence-position cases this branch's review asked for, and identical files merge without a conflict. * Make fixing what watching does, and off a per-repository answer Watching a pull request nobody fixes is a queue that reports work and does none. That is what happened: the drain was installed for one repository, every other repository in the fleet kept collecting review feedback, and one PR reached 36 unresolved threads with nothing to drain them. Nothing was broken — nothing was watching for fixes there. So dispatch is the default, and three things turn it off, each at its own scale: --no-dispatch for one run, `crq drain off <repo>` for one repository, and having no fix command configured at all. The last one observes with a log line instead of refusing to start: making the default setting break the plain command is not a default. `crq drain off` stops FIXING, not watching. The pull request is still observed and still reviewed, so feedback keeps arriving for a person to act on — the switch is about who writes the code, not about whether crq looks. It lives in the state ref alongside the reviewer overrides, for the reason recorded there: the daemon has no checkout of what it watches, and a daemon and an agent reading different configurations while writing one ref is a class of divergence worth not having. Also fixes what widening the fleet exposed within a minute: watchPass appended to the caller's repository slice. `crq watch -- <cmd>` splits argv at "--", so the flag half keeps capacity reaching into the command half and fs.Args() is a sub-slice of it — filling an empty repository list wrote the repository names over the fix command, and every dispatch in the fleet died with "fork/exec kristofferr/coderabbit-queue: no such file or directory". Fixed on both sides: the pass copies rather than appends, and the CLI caps the capacity it hands out. The drain-health alert caught it in three attempts, and no attempt was burned — a command that never started gives its attempt back. * Stop the push gate creating states a session cannot leave The review of the gate found three more ways a dispatched session can never reach `push`, all the same shape as the one that stranded a commit an hour ago. Each verified against the code first. The gate itself was too narrow. Its purpose is to keep a head from moving under a reviewer who is reading THAT commit, and that is `hold`/`wait` — not every action other than `push`. `fix` is where a session that has just made changes usually lands, and refusing to push there is what turned each of the following into a deadlock rather than a delay. The prompt now pushes unless crq says a reviewer is mid-read. reviewRequested counted a dispatch hold as a requested review. ClaimDispatch mirrors a queued round into awaiting_retry so binaries that do not understand the claim still refuse to fire; reading that back as "a review was requested" told the holding session to wait for a reviewer that could not be asked — the claim is what makes the round ineligible — while its own heartbeat extended the window it was waiting on. DispatchHoldPhase records what the round was before the hold, which is the phase the question is about. claimDispatch deduped the head on ANY finding. A Codex or Bugbot finding says a co-reviewer looked, and they spend no account quota: marking the round completed on that evidence retires a primary review nobody ever requested, and Pump can then never fire it. Dedupe now needs the metered primary's own review; a co-reviewer finding leaves the round adoptable, so the queue can still buy the review that is actually missing. And `crq drain <typo>` listed instead of failing — an answer to a question nobody asked, in place of the instruction that was meant. * Push before resolving, because only one order is recoverable Resolving first and pushing second means a session that dies in between leaves GitHub threads marked addressed for a commit that is on no branch. The next pass filters those threads out, finds no findings, dispatches nobody — and the fix is stranded for good while the pull request records it as done. That is not a hypothetical: a session did exactly this an hour ago, reporting "resolved both review threads" and "Not pushed". Pushing first is recoverable in both directions. Die before the push and nothing was claimed to be fixed, so the next session redoes it. Die after it and the fix is on the branch, where the next review judges it on its merits. Nothing needed to move for this: with the gate now reading `hold`/`wait` rather than demanding `push`, a session that has just made changes is free to push at `fix`, which is where it lands. The steps only had to be swapped. * Fix review dispatch race conditions * Harden unattended review dispatch * Fix unattended review drain safeguards * Fix unattended dispatch review findings * Make CRQ_EXCLUDE mean it everywhere autoReviewPass has always honoured the denylist; watchPass never looked at it. So the one setting that reads like "crq does not go here" covered half of what crq does — reviews stopped for an excluded repository and the watcher carried on observing it and starting fix sessions in it. Half a setting is worse than none: it is the kind an operator sets, verifies by watching reviews stop, and reasonably believes. Excluding a repository now excludes it from the pass too, and the documentation says so rather than naming only autoreview. * Carry the denylist into the installed unit too drainEnv wrote CRQ_REPOS and not CRQ_EXCLUDE, so an install could produce a service that watches a repository the operator had excluded — the same shape as the quota timings this file already learned to carry: the service does not inherit the shell that installed it, so anything that changes what it does has to be written down. --------- Co-authored-by: kristofferR <kristofferR@users.noreply.github.com>

chatgpt-codex-connectorBot reviewed Jul 26, 2026
View reviewed changes

Comment threadinternal/crq/observe.go

kristofferR mentioned this pull request Jul 26, 2026
Choose which bots review which project #52
Closed

coderabbitaiBot reviewed Jul 26, 2026
View reviewed changes

Comment threadinternal/crq/observe_config_test.go Outdated

Scope the config-threading guard to observe itself
9853acf
Counting s.cfg. across the whole file would fail on an unrelated method, or on the words appearing in a comment, so the guard would break for reasons that have nothing to do with what it protects. It reads the function's own body now.

kristofferR merged commit fbd4907 into mainJul 26, 2026
2 checks passed

kristofferR deleted the refactor/reviewer-config-as-value branch July 26, 2026 20:33

This was referenced Jul 26, 2026
Choose which bots review which project #57
Merged
v3 roadmap: one instruction, one reviewer list, one owner of the wait #42
Open

coderabbitaiBot mentioned this pull request Jul 27, 2026
Let an agent account for a finding GitHub cannot close #48
Merged

Conversation

kristofferR commented Jul 26, 2026• edited by coderabbitai BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What changed

Verification

Summary by CodeRabbit

Uh oh!

coderabbitaiBot commented Jul 26, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

chatgpt-codex-connectorBot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

coderabbitaiBot commented Jul 26, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kristofferR commented Jul 26, 2026

Uh oh!

kristofferR commented Jul 26, 2026

Uh oh!

chatgpt-codex-connectorBot commented Jul 26, 2026

Uh oh!

coderabbitaiBot commented Jul 26, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kristofferR commented Jul 26, 2026

Uh oh!

coderabbitaiBot commented Jul 26, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kristofferR commented Jul 26, 2026

Uh oh!

coderabbitaiBot commented Jul 26, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitaiBot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kristofferR commented Jul 26, 2026

Uh oh!

kristofferR commented Jul 26, 2026

Uh oh!

coderabbitaiBot commented Jul 26, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chatgpt-codex-connectorBot commented Jul 26, 2026

Uh oh!

kristofferR commented Jul 26, 2026

Uh oh!

coderabbitaiBot commented Jul 26, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

kristofferR commented Jul 26, 2026•
edited by coderabbitai Bot
Loading

coderabbitaiBot commented Jul 26, 2026•
edited
Loading

coderabbitaiBot commented Jul 26, 2026•
edited
Loading

coderabbitaiBot commented Jul 26, 2026•
edited
Loading

coderabbitaiBot commented Jul 26, 2026•
edited
Loading

coderabbitaiBot commented Jul 26, 2026•
edited
Loading

coderabbitaiBot commented Jul 26, 2026•
edited
Loading

coderabbitaiBot commented Jul 26, 2026•
edited
Loading