Give crq its own place on disk for a repository#53
Conversation
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.
|
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:
📝 WalkthroughWalkthroughChangesWorkspace checkout lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Service
participant Workspace
participant Mirror
participant Checkout
Service->>Workspace: create configured workspace
Service->>Workspace: Checkout(repo, pr, sha)
Workspace->>Mirror: create or refresh bare mirror
Mirror-->>Workspace: mirror directory
Workspace->>Checkout: create detached worktree
Checkout-->>Service: checkout directory
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8361991202
ℹ️ 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".
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0506d379b
ℹ️ 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".
✅ Action performedReview finished.
|
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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bff9ecdf2
ℹ️ 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 review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
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.
|
@codex review |
|
@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: 16c60043c0
ℹ️ 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 review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/workspace.go`:
- Around line 306-330: Convert removeWorktree from a Workspace method to a
package-level function, since it does not use Workspace state. Update
Checkout.Remove to call the function directly instead of constructing
Workspace{}; preserve the existing removal and pruning behavior.
- Around line 199-244: Harden the git worktree creation in Workspace.Checkout by
adding a `--` argument separator before the positional dir and sha arguments in
the `w.git` call. Preserve the existing detached worktree behavior and error
handling while ensuring neither argument can be interpreted as a git option.
- Around line 363-377: The new Service.workspace and gitToken declarations are
unused. Wire Service.workspace into the dispatch flow so workspace construction
uses the configured root and resolved git token, ensuring gitToken is reached;
if dispatch integration is intentionally deferred, remove both declarations
instead.
🪄 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: 6b244612-27d6-4be8-b1d6-680b3fe32398
📒 Files selected for processing (6)
internal/crq/config.gointernal/crq/next.gointernal/crq/target.gointernal/crq/workspace.gointernal/crq/workspace_test.gointernal/gh/github.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
internal/gh/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Keep GitHub REST/GraphQL transport and the GitHub REST quota concept (
Throttle,ThrottleWait,IsThrottled) ininternal/gh; matching check runs to bots must use dialect classification rather than gh logic.
Files:
internal/gh/github.go
internal/crq/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Keep
internal/crqlimited to orchestration and effects wiring;service.gois the only effects executor for CAS writes andPostIssueComment, andDryRunmust write nothing.
Files:
internal/crq/config.gointernal/crq/next.gointernal/crq/target.gointernal/crq/workspace_test.gointernal/crq/workspace.go
internal/crq/config.go
📄 CodeRabbit inference engine (AGENTS.md)
Add a default entry in
parseCoBotswhen a new co-reviewer should be enabled by default.
Files:
internal/crq/config.go
🪛 golangci-lint (2.12.2)
internal/crq/workspace_test.go
[medium] 17-17: G301: Expect directory permissions to be 0750 or less
(gosec)
[medium] 31-31: G306: Expect WriteFile permissions to be 0600 or less
(gosec)
internal/crq/workspace.go
[medium] 129-129: G302: Expect file permissions to be 0600 or less
(gosec)
[medium] 346-346: G204: Subprocess launched with variable
(gosec)
[error] 296-296: error is not nil (line 294) but it returns nil
(nilerr)
[error] 366-366: func (*Service).workspace is unused
(unused)
[error] 377-377: func gitToken is unused
(unused)
🔇 Additional comments (10)
internal/crq/workspace.go (5)
1-112: LGTM!
114-197: LGTM!
246-304: LGTM!
332-361: LGTM! (gosec's G204 here is expected — runninggitwith dynamic args is this file's whole purpose.)
379-389: LGTM!internal/crq/workspace_test.go (1)
1-321: LGTM!internal/crq/config.go (1)
50-59: LGTM!Also applies to: 184-184
internal/gh/github.go (1)
170-174: LGTM!internal/crq/next.go (1)
4-5: LGTM!Also applies to: 291-296, 315-324
internal/crq/target.go (1)
5-7: LGTM!Also applies to: 119-123
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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 170892387b
ℹ️ 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 review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
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.
|
@codex review |
|
@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: 67408ca874
ℹ️ 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".
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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 036c830c90
ℹ️ 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 review |
✅ Action performedReview finished.
|
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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ca6e83fa7
ℹ️ 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".
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.
|
@codex review |
Roadmap item I, and the groundwork
crq watch --dispatch(item J) has been waiting on.The gap
Every git call in crq runs in the process's own working directory:
That is right for a command an agent types from its working copy. It is useless for the daemon,
which has no checkout of any repository it reviews — which is exactly why dispatch has nothing to
stand on. There are no clone, fetch, worktree or push calls anywhere in the repository today.
Workspace
A bare mirror per repository, fetched rather than re-cloned, and a throwaway worktree per head:
Three decisions worth stating:
Detached HEAD. This is a place to inspect and build, not a branch to commit to by accident. A
caller that means to write creates its own branch.
Mirrors are reused, worktrees are not. The expensive clone happens once per repository rather
than once per dispatch; a worktree left behind by a killed process is replaced, since the head it
holds is stale anyway.
Credentials are not crq's business. The remote is the ordinary https URL, so whatever credential
helper the host already has (
gh auth setup-gitwrites one) supplies the token. crq never holds asecret it would then have to avoid logging.
Also
One
gitDir(ctx, dir, args...)runs every git command now, and folds stderr into the error —exit status 128on its own has never told anybody what went wrong.localWorktakes a directorythrough
Config.WorkDir, so a caller working in a worktree it made can say so; that is a configfield rather than a copied Service, because Service carries rotation counters a copy would silently
fork.
Mirror/Checkoutrefuse a repo string that is not exactlyowner/name— those become pathsegments, and
owner/../../etcmust not be joined onto the workspace root.Verification
The tests build a real git repository and clone it, because what is under test is whether the git
invocations are right, not whether a mock returns what I told it to. They cover: clone then fetch
into the same mirror, a worktree with the committed content at the right SHA, detached HEAD,
re-checkout over a leftover worktree, removal, and removal of an already-removed worktree.
Path-escape names are refused, and the error-message test asserts git's own stderr survives.
gofmt -l .clean,go vet ./...clean,go test ./... -count=1green. Local preflight: 0 findings.Ref #42
Summary by CodeRabbit
New Features
Improvements