From 8361991202b7963610ea080c4711b3b2e4475cd7 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 19:16:24 +0200 Subject: [PATCH 01/17] Give crq its own place on disk for a repository MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/crq/config.go | 7 +- internal/crq/next.go | 11 +- internal/crq/target.go | 9 +- internal/crq/workspace.go | 209 +++++++++++++++++++++++++++++++++ internal/crq/workspace_test.go | 121 +++++++++++++++++++ 5 files changed, 345 insertions(+), 12 deletions(-) create mode 100644 internal/crq/workspace.go create mode 100644 internal/crq/workspace_test.go diff --git a/internal/crq/config.go b/internal/crq/config.go index 98d6cdf..9d24f26 100644 --- a/internal/crq/config.go +++ b/internal/crq/config.go @@ -47,7 +47,12 @@ type Config struct { // Reviewers is the single description of who reviews and what they cost. // Bot / RequiredBots / FeedbackBots / CoBots above are DERIVED from it and // kept only so existing consumers keep compiling; new code should read this. - Reviewers []Reviewer + Reviewers []Reviewer + // WorkDir is the checkout the local-work probe inspects. Empty means the + // process's own directory, which is what an agent running crq from its + // working copy means. Set programmatically by a caller working in a + // worktree it made, since that caller is not standing in one. + WorkDir string RateLimitCommand string RateLimitMarker string CalibrationMarker string diff --git a/internal/crq/next.go b/internal/crq/next.go index d72155a..1ee0d81 100644 --- a/internal/crq/next.go +++ b/internal/crq/next.go @@ -2,7 +2,6 @@ package crq import ( "context" - "os/exec" "strings" "time" @@ -293,7 +292,7 @@ func (s *Service) checkLocalWork(ctx context.Context, repos []string, head, head if s.localWorkFn != nil { return s.localWorkFn(ctx, head) } - return localWork(ctx, repos, head, headRef) + return localWork(ctx, s.cfg.WorkDir, repos, head, headRef) } // localWork reports whether the working copy holds changes the PR head does not @@ -313,13 +312,15 @@ func (s *Service) checkLocalWork(ctx context.Context, repos []string, head, head // `done` rather than `push`. That is the safe direction: `push` is only ever // emitted once the head is already released, so a missed one costs one extra // call, while a spurious `hold` would stall the loop. -func localWork(ctx context.Context, repos []string, head, headRef string) (bool, string) { +// dir is the checkout to inspect; "" means the process's own directory, +// which is what an agent running crq from its working copy means. +func localWork(ctx context.Context, dir string, repos []string, head, headRef string) (bool, string) { git := func(args ...string) (string, bool) { - out, err := exec.CommandContext(ctx, "git", args...).Output() + out, err := gitDir(ctx, dir, args...) if err != nil { return "", false } - return strings.TrimSpace(string(out)), true + return out, true } if _, ok := git("rev-parse", "--is-inside-work-tree"); !ok { return false, "not run inside a git checkout" diff --git a/internal/crq/target.go b/internal/crq/target.go index 36ed7d3..d577191 100644 --- a/internal/crq/target.go +++ b/internal/crq/target.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "net/url" - "os/exec" "strings" ) @@ -117,10 +116,8 @@ func remoteSlugs(remotes string) (repos, owners []string) { return repos, owners } +// gitIn runs git in the process's own working directory, which is what target +// inference is about: the checkout the caller is standing in. func gitIn(ctx context.Context, args ...string) (string, error) { - out, err := exec.CommandContext(ctx, "git", args...).Output() - if err != nil { - return "", err - } - return strings.TrimSpace(string(out)), nil + return gitDir(ctx, "", args...) } diff --git a/internal/crq/workspace.go b/internal/crq/workspace.go new file mode 100644 index 0000000..455cb07 --- /dev/null +++ b/internal/crq/workspace.go @@ -0,0 +1,209 @@ +package crq + +import ( + "context" + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" +) + +// Workspace is crq's own place on disk for a repository. +// +// Everything crq does with git today assumes it was RUN inside the checkout it +// cares about: localWork shells out with no directory, target inference reads +// the current branch. That is fine for a command an agent types from its own +// working copy, and useless for the daemon, which has no checkout of any of the +// repositories it reviews. +// +// A Workspace separates the two: a bare mirror per repository, fetched rather +// than re-cloned, and a throwaway worktree per head. Nothing here reads or +// writes the process's working directory. +// +// Credentials are deliberately not crq's business. The remote is the ordinary +// https URL, so whatever git credential helper the host already uses — `gh auth +// setup-git` writes one — supplies the token. crq never holds a secret it would +// then have to avoid logging. +type Workspace struct { + // Root holds the mirrors and worktrees. Empty means the default cache + // location (see DefaultWorkspaceRoot). + Root string +} + +// DefaultWorkspaceRoot is $XDG_CACHE_HOME/crq (or ~/.cache/crq). +func DefaultWorkspaceRoot() (string, error) { + if dir := strings.TrimSpace(os.Getenv("CRQ_WORKSPACE")); dir != "" { + return dir, nil + } + cache, err := os.UserCacheDir() + if err != nil { + return "", err + } + return filepath.Join(cache, "crq"), nil +} + +func (w Workspace) root() (string, error) { + if strings.TrimSpace(w.Root) != "" { + return w.Root, nil + } + return DefaultWorkspaceRoot() +} + +// mirrorPath is where repo's bare mirror lives. The owner and name are path +// segments, so a repo string that is not exactly "owner/name" is refused rather +// than allowed to escape the root. +func (w Workspace) mirrorPath(repo string) (string, error) { + root, err := w.root() + if err != nil { + return "", err + } + owner, name, ok := splitRepo(repo) + if !ok { + return "", fmt.Errorf("repo must be owner/name, got %q", repo) + } + return filepath.Join(root, "mirrors", owner, name+".git"), nil +} + +// splitRepo splits owner/name, rejecting anything that could traverse a path. +func splitRepo(repo string) (owner, name string, ok bool) { + owner, name, ok = strings.Cut(NormalizeRepo(repo), "/") + if !ok || owner == "" || name == "" { + return "", "", false + } + for _, part := range []string{owner, name} { + if part == "." || part == ".." || strings.ContainsAny(part, `/\`) { + return "", "", false + } + } + return owner, name, true +} + +// Mirror makes sure repo's bare mirror exists and is up to date, and returns its +// path. It clones on first use and fetches afterwards — a mirror is reused +// across PRs and across rounds, so the expensive clone happens once per +// repository rather than once per dispatch. +func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { + path, err := w.mirrorPath(repo) + if err != nil { + return "", err + } + if _, err := os.Stat(filepath.Join(path, "HEAD")); err == nil { + if _, err := gitDir(ctx, path, "fetch", "--prune", "origin"); err != nil { + return "", fmt.Errorf("fetching %s: %w", repo, err) + } + return path, nil + } else if !errors.Is(err, os.ErrNotExist) { + return "", err + } + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return "", err + } + if _, err := gitDir(ctx, "", "clone", "--mirror", w.remoteURL(repo), path); err != nil { + return "", fmt.Errorf("cloning %s: %w", repo, err) + } + return path, nil +} + +// remoteURL is the URL crq clones from. CRQ_REMOTE_BASE exists so a test can +// point at a local path; in production it is github.com over https, which is +// what the host's credential helper is configured for. +func (w Workspace) remoteURL(repo string) string { + base := strings.TrimSpace(os.Getenv("CRQ_REMOTE_BASE")) + if base == "" { + return "https://github.com/" + NormalizeRepo(repo) + ".git" + } + return strings.TrimRight(base, "/") + "/" + NormalizeRepo(repo) +} + +// Checkout is a worktree at one commit, and the directory git commands for that +// PR run in. +type Checkout struct { + Dir string + Repo string + PR int + mirror string +} + +// Checkout creates a worktree of repo at sha, on a detached HEAD. +// +// Detached on purpose: 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. +func (w Workspace) Checkout(ctx context.Context, repo string, pr int, sha string) (Checkout, error) { + if strings.TrimSpace(sha) == "" { + return Checkout{}, errors.New("checkout needs a commit") + } + mirror, err := w.Mirror(ctx, repo) + if err != nil { + return Checkout{}, err + } + root, err := w.root() + if err != nil { + return Checkout{}, err + } + owner, name, ok := splitRepo(repo) + if !ok { + return Checkout{}, fmt.Errorf("repo must be owner/name, got %q", repo) + } + dir := filepath.Join(root, "work", fmt.Sprintf("%s-%s-%d", owner, name, pr)) + // A worktree left behind by a killed process would make this fail forever, + // so replace rather than reuse: the head it holds is probably stale anyway. + if err := w.removeWorktree(ctx, mirror, dir); err != nil { + return Checkout{}, err + } + if err := os.MkdirAll(filepath.Dir(dir), 0o755); err != nil { + return Checkout{}, err + } + if _, err := gitDir(ctx, mirror, "worktree", "add", "--detach", dir, sha); err != nil { + return Checkout{}, fmt.Errorf("checking out %s@%s: %w", repo, shortSHA(sha), err) + } + return Checkout{Dir: dir, Repo: NormalizeRepo(repo), PR: pr, mirror: mirror}, nil +} + +// Remove deletes the worktree. Safe to call on an already-removed one. +func (c Checkout) Remove(ctx context.Context) error { + if c.mirror == "" || c.Dir == "" { + return nil + } + return Workspace{}.removeWorktree(ctx, c.mirror, c.Dir) +} + +func (w Workspace) removeWorktree(ctx context.Context, mirror, dir string) error { + if _, err := os.Stat(dir); errors.Is(err, os.ErrNotExist) { + // Still prune: git may hold a record of a directory somebody deleted. + _, _ = gitDir(ctx, mirror, "worktree", "prune") + return nil + } + if _, err := gitDir(ctx, mirror, "worktree", "remove", "--force", dir); err != nil { + // Fall back to deleting it ourselves; a stale registration is pruned. + if rmErr := os.RemoveAll(dir); rmErr != nil { + return rmErr + } + } + _, _ = gitDir(ctx, mirror, "worktree", "prune") + return nil +} + +// Git runs a git command inside this checkout. +func (c Checkout) Git(ctx context.Context, args ...string) (string, error) { + return gitDir(ctx, c.Dir, args...) +} + +// gitDir runs git in dir ("" means the process's own directory) and returns its +// trimmed stdout. Stderr is folded into the error, because "exit status 128" on +// its own has never told anybody what went wrong. +func gitDir(ctx context.Context, dir string, args ...string) (string, error) { + cmd := exec.CommandContext(ctx, "git", args...) + cmd.Dir = dir + var stderr strings.Builder + cmd.Stderr = &stderr + out, err := cmd.Output() + if err != nil { + if detail := strings.TrimSpace(stderr.String()); detail != "" { + return "", fmt.Errorf("git %s: %w: %s", strings.Join(args, " "), err, detail) + } + return "", fmt.Errorf("git %s: %w", strings.Join(args, " "), err) + } + return strings.TrimSpace(string(out)), nil +} diff --git a/internal/crq/workspace_test.go b/internal/crq/workspace_test.go new file mode 100644 index 0000000..a07d4a4 --- /dev/null +++ b/internal/crq/workspace_test.go @@ -0,0 +1,121 @@ +package crq + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" +) + +// originRepo builds a real repository on disk to clone from, so these tests +// exercise git itself rather than a mock of it — the whole point of this code is +// that the git invocations are right. +func originRepo(t *testing.T, dir string) (sha string) { + t.Helper() + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + run := func(args ...string) string { + t.Helper() + out, err := gitDir(context.Background(), dir, args...) + if err != nil { + t.Fatalf("git %s: %v", strings.Join(args, " "), err) + } + return out + } + run("init", "--initial-branch=main") + run("config", "user.email", "test@example.invalid") + run("config", "user.name", "crq test") + if err := os.WriteFile(filepath.Join(dir, "README.md"), []byte("one\n"), 0o644); err != nil { + t.Fatal(err) + } + run("add", "README.md") + run("commit", "-m", "first") + return run("rev-parse", "HEAD") +} + +// crq's daemon has no checkout of any repository it reviews, so it needs its own +// place on disk. This covers the whole shape: clone once, fetch after, worktree +// at a commit, and clean up. +func TestWorkspaceChecksOutAHeadWithoutACheckout(t *testing.T) { + // The remote base plus "owner/name" has to resolve to a real repository, so + // lay the origin out the way GitHub does. + base := t.TempDir() + repo := "owner/thing" + sha := originRepo(t, filepath.Join(base, repo)) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + + mirror, err := ws.Mirror(ctx, repo) + if err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(mirror, "HEAD")); err != nil { + t.Fatalf("mirror is not a git directory: %v", err) + } + + // Second call fetches into the same mirror rather than cloning again: a + // mirror is reused across PRs and rounds, so the clone happens once. + again, err := ws.Mirror(ctx, repo) + if err != nil { + t.Fatal(err) + } + if again != mirror { + t.Errorf("second Mirror = %q, want the same path %q", again, mirror) + } + + co, err := ws.Checkout(ctx, repo, 7, sha) + if err != nil { + t.Fatal(err) + } + if body, err := os.ReadFile(filepath.Join(co.Dir, "README.md")); err != nil || string(body) != "one\n" { + t.Fatalf("worktree content = %q err=%v, want the committed file", body, err) + } + // Detached on purpose: this is a place to inspect, not a branch to commit to + // by accident. + if branch, err := co.Git(ctx, "rev-parse", "--abbrev-ref", "HEAD"); err != nil || branch != "HEAD" { + t.Errorf("HEAD = %q err=%v, want a detached checkout", branch, err) + } + if head, err := co.Git(ctx, "rev-parse", "HEAD"); err != nil || head != sha { + t.Errorf("checked out %q, want %q", head, sha) + } + + // A worktree left by a killed process must not wedge the next attempt. + if _, err := ws.Checkout(ctx, repo, 7, sha); err != nil { + t.Fatalf("re-checkout over an existing worktree: %v", err) + } + if err := co.Remove(ctx); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(co.Dir); !os.IsNotExist(err) { + t.Errorf("worktree still present after Remove: %v", err) + } + if err := co.Remove(ctx); err != nil { + t.Errorf("removing an already-removed worktree must be harmless, got %v", err) + } +} + +// A repo name is turned into path segments, so anything that could climb out of +// the workspace root has to be refused rather than joined. +func TestWorkspaceRefusesRepoNamesThatEscape(t *testing.T) { + ws := Workspace{Root: t.TempDir()} + for _, repo := range []string{"", "noslash", "../etc/passwd", "owner/../../etc", "owner/", "/name"} { + if _, err := ws.mirrorPath(repo); err == nil { + t.Errorf("mirrorPath(%q) was accepted; it must be refused", repo) + } + } +} + +// The error has to say what git said. "exit status 128" alone has never told +// anybody what went wrong. +func TestGitErrorsCarryStderr(t *testing.T) { + _, err := gitDir(context.Background(), t.TempDir(), "rev-parse", "HEAD") + if err == nil { + t.Fatal("expected an error outside a repository") + } + if !strings.Contains(err.Error(), "rev-parse") || !strings.Contains(strings.ToLower(err.Error()), "git") { + t.Errorf("error %q does not name the command that failed", err) + } +} From b0506d379b6a2cbc8e951b7512ac527c34e30486 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 19:31:48 +0200 Subject: [PATCH 02/17] Keep one checkout from destroying another, and private source private MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/crq/config.go | 5 ++ internal/crq/workspace.go | 138 +++++++++++++++++++++++++++++---- internal/crq/workspace_test.go | 93 ++++++++++++++++++++++ 3 files changed, 221 insertions(+), 15 deletions(-) diff --git a/internal/crq/config.go b/internal/crq/config.go index 9d24f26..719d5a7 100644 --- a/internal/crq/config.go +++ b/internal/crq/config.go @@ -48,6 +48,10 @@ type Config struct { // Bot / RequiredBots / FeedbackBots / CoBots above are DERIVED from it and // kept only so existing consumers keep compiling; new code should read this. Reviewers []Reviewer + // WorkspaceRoot holds crq's own mirrors and worktrees. Read here rather than + // from the process environment, so a value in ~/.config/crq/env — the + // documented place for crq settings — is actually used. + WorkspaceRoot string // WorkDir is the checkout the local-work probe inspects. Empty means the // process's own directory, which is what an agent running crq from its // working copy means. Set programmatically by a caller working in a @@ -177,6 +181,7 @@ func LoadConfig() (Config, error) { AutoReviewMaxScan: intEnv(env, "CRQ_AUTOREVIEW_MAX_SCAN", 400), LeaderTTL: durationEnv(env, "CRQ_LEADER_TTL", 3*time.Minute), FiredMax: intEnv(env, "CRQ_FIRED_MAX", 500), + WorkspaceRoot: env["CRQ_WORKSPACE"], NoOpen: env["CRQ_NO_OPEN"] != "", DryRun: env["CRQ_DRY_RUN"] == "1", FeedbackWaitTimeout: durationEnv(env, "CRQ_FEEDBACK_WAIT_TIMEOUT", 20*time.Minute), diff --git a/internal/crq/workspace.go b/internal/crq/workspace.go index 455cb07..12fcc22 100644 --- a/internal/crq/workspace.go +++ b/internal/crq/workspace.go @@ -7,6 +7,7 @@ import ( "os" "os/exec" "path/filepath" + "strconv" "strings" ) @@ -30,6 +31,11 @@ type Workspace struct { // Root holds the mirrors and worktrees. Empty means the default cache // location (see DefaultWorkspaceRoot). Root string + // Token authenticates clones and fetches. Empty falls back to whatever git + // credential helper the host already has, which is the right answer on a + // developer machine; a daemon configured with GITHUB_TOKEN alone has no + // helper, and git does not read that variable by itself. + Token string } // DefaultWorkspaceRoot is $XDG_CACHE_HOME/crq (or ~/.cache/crq). @@ -44,6 +50,21 @@ func DefaultWorkspaceRoot() (string, error) { return filepath.Join(cache, "crq"), nil } +// git runs a git command for this workspace, injecting credentials when a token +// is configured. +// +// The token travels in the ENVIRONMENT, never in argv: the helper below is a +// shell snippet that reads it, so a process listing, a log line and this +// package's own error strings all carry the snippet and never the secret. +func (w Workspace) git(ctx context.Context, dir string, args ...string) (string, error) { + if strings.TrimSpace(w.Token) == "" { + return gitDir(ctx, dir, args...) + } + const helper = `!f() { test "$1" = get && printf 'username=x-access-token\npassword=%s\n' "$CRQ_GIT_TOKEN"; }; f` + full := append([]string{"-c", "credential.helper=", "-c", "credential.helper=" + helper}, args...) + return gitEnv(ctx, dir, []string{"CRQ_GIT_TOKEN=" + w.Token}, full...) +} + func (w Workspace) root() (string, error) { if strings.TrimSpace(w.Root) != "" { return w.Root, nil @@ -89,20 +110,45 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { if err != nil { return "", err } + // 0700, not the umask's 0755: a mirror of a private repository is private + // source, and on a shared host the default would let any local user read it. + if root, rerr := w.root(); rerr == nil { + if err := os.MkdirAll(root, 0o700); err != nil { + return "", err + } + if err := os.Chmod(root, 0o700); err != nil { + return "", err + } + } if _, err := os.Stat(filepath.Join(path, "HEAD")); err == nil { - if _, err := gitDir(ctx, path, "fetch", "--prune", "origin"); err != nil { + if _, err := w.git(ctx, path, "fetch", "--prune", "origin"); err != nil { return "", fmt.Errorf("fetching %s: %w", repo, err) } return path, nil } else if !errors.Is(err, os.ErrNotExist) { return "", err } - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + return "", err + } + // Clone somewhere else and move it into place, so two workers starting on the + // same repository at once cannot clone into one directory — the second would + // fail with "destination path already exists" and take the caller with it. + staging, err := os.MkdirTemp(filepath.Dir(path), ".clone-") + if err != nil { return "", err } - if _, err := gitDir(ctx, "", "clone", "--mirror", w.remoteURL(repo), path); err != nil { + defer os.RemoveAll(staging) + pending := filepath.Join(staging, "mirror.git") + if _, err := w.git(ctx, "", "clone", "--mirror", w.remoteURL(repo), pending); err != nil { return "", fmt.Errorf("cloning %s: %w", repo, err) } + if err := os.Rename(pending, path); err != nil { + if _, statErr := os.Stat(filepath.Join(path, "HEAD")); statErr == nil { + return path, nil // somebody else won the race; their mirror is as good + } + return "", err + } return path, nil } @@ -124,6 +170,10 @@ type Checkout struct { Repo string PR int mirror string + // token makes this handle's directory its own. Two checkouts of one PR + // otherwise share a path, and a deferred Remove on the older handle deletes + // the newer one's worktree out from under it. + token string } // Checkout creates a worktree of repo at sha, on a detached HEAD. @@ -138,32 +188,64 @@ func (w Workspace) Checkout(ctx context.Context, repo string, pr int, sha string if err != nil { return Checkout{}, err } - root, err := w.root() + prDir, err := w.workPath(repo, pr) if err != nil { return Checkout{}, err } - owner, name, ok := splitRepo(repo) - if !ok { - return Checkout{}, fmt.Errorf("repo must be owner/name, got %q", repo) - } - dir := filepath.Join(root, "work", fmt.Sprintf("%s-%s-%d", owner, name, pr)) - // A worktree left behind by a killed process would make this fail forever, - // so replace rather than reuse: the head it holds is probably stale anyway. - if err := w.removeWorktree(ctx, mirror, dir); err != nil { + // Anything already here belongs to an earlier checkout of this PR, which is + // finished or dead either way. Clearing it keeps the generation directories + // from accumulating without letting a live handle delete a newer sibling. + if err := w.clearWork(ctx, mirror, prDir); err != nil { return Checkout{}, err } - if err := os.MkdirAll(filepath.Dir(dir), 0o755); err != nil { + token := randomToken() + dir := filepath.Join(prDir, token) + if err := os.MkdirAll(prDir, 0o700); err != nil { return Checkout{}, err } if _, err := gitDir(ctx, mirror, "worktree", "add", "--detach", dir, sha); err != nil { return Checkout{}, fmt.Errorf("checking out %s@%s: %w", repo, shortSHA(sha), err) } - return Checkout{Dir: dir, Repo: NormalizeRepo(repo), PR: pr, mirror: mirror}, nil + return Checkout{Dir: dir, Repo: NormalizeRepo(repo), PR: pr, mirror: mirror, token: token}, nil +} + +// workPath is the directory holding this PR's checkouts. Owner and name stay +// separate path components: joined with a dash, "a-b/c" and "a/b-c" would +// collide, and one repository's cleanup would delete the other's live worktree. +func (w Workspace) workPath(repo string, pr int) (string, error) { + root, err := w.root() + if err != nil { + return "", err + } + owner, name, ok := splitRepo(repo) + if !ok { + return "", fmt.Errorf("repo must be owner/name, got %q", repo) + } + return filepath.Join(root, "work", owner, name, strconv.Itoa(pr)), nil +} + +// clearWork removes every checkout under prDir and deregisters it. +func (w Workspace) clearWork(ctx context.Context, mirror, prDir string) error { + entries, err := os.ReadDir(prDir) + if errors.Is(err, os.ErrNotExist) { + return nil + } + if err != nil { + return err + } + for _, entry := range entries { + if err := w.removeWorktree(ctx, mirror, filepath.Join(prDir, entry.Name())); err != nil { + return err + } + } + return nil } // Remove deletes the worktree. Safe to call on an already-removed one. func (c Checkout) Remove(ctx context.Context) error { - if c.mirror == "" || c.Dir == "" { + if c.mirror == "" || c.Dir == "" || filepath.Base(c.Dir) != c.token { + // Not this handle's directory: a later checkout replaced it, and removing + // it would delete a worktree somebody else is using. return nil } return Workspace{}.removeWorktree(ctx, c.mirror, c.Dir) @@ -194,8 +276,16 @@ func (c Checkout) Git(ctx context.Context, args ...string) (string, error) { // trimmed stdout. Stderr is folded into the error, because "exit status 128" on // its own has never told anybody what went wrong. func gitDir(ctx context.Context, dir string, args ...string) (string, error) { + return gitEnv(ctx, dir, nil, args...) +} + +// gitEnv is gitDir with extra environment entries. +func gitEnv(ctx context.Context, dir string, env []string, args ...string) (string, error) { cmd := exec.CommandContext(ctx, "git", args...) cmd.Dir = dir + if len(env) > 0 { + cmd.Env = append(os.Environ(), env...) + } var stderr strings.Builder cmd.Stderr = &stderr out, err := cmd.Output() @@ -207,3 +297,21 @@ func gitDir(ctx context.Context, dir string, args ...string) (string, error) { } return strings.TrimSpace(string(out)), nil } + +// workspace is crq's workspace as this configuration describes it: the root from +// the config file (not just the process environment) and the token crq already +// resolved for the API, so a daemon with GITHUB_TOKEN alone can still clone. +func (s *Service) workspace() Workspace { + return Workspace{Root: s.cfg.WorkspaceRoot, Token: gitToken()} +} + +// gitToken is the token to authenticate git with, or "" to leave git to the +// host's own credential helper. +func gitToken() string { + for _, name := range []string{"GITHUB_TOKEN", "GH_TOKEN"} { + if value := strings.TrimSpace(os.Getenv(name)); value != "" { + return value + } + } + return "" +} diff --git a/internal/crq/workspace_test.go b/internal/crq/workspace_test.go index a07d4a4..6a14e58 100644 --- a/internal/crq/workspace_test.go +++ b/internal/crq/workspace_test.go @@ -119,3 +119,96 @@ func TestGitErrorsCarryStderr(t *testing.T) { t.Errorf("error %q does not name the command that failed", err) } } + +// Four ways a workspace could destroy or expose something it should not. +func TestWorkspaceIsolatesCheckouts(t *testing.T) { + base := t.TempDir() + sha := originRepo(t, filepath.Join(base, "owner/thing")) + t.Setenv("CRQ_REMOTE_BASE", base) + root := t.TempDir() + ws := Workspace{Root: root} + ctx := context.Background() + + // A stale handle must not delete the checkout that replaced it. + first, err := ws.Checkout(ctx, "owner/thing", 9, sha) + if err != nil { + t.Fatal(err) + } + second, err := ws.Checkout(ctx, "owner/thing", 9, sha) + if err != nil { + t.Fatal(err) + } + if first.Dir == second.Dir { + t.Fatal("two checkouts of one PR share a directory, so either can delete the other") + } + if err := first.Remove(ctx); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(second.Dir); err != nil { + t.Errorf("the live checkout was removed by a stale handle: %v", err) + } + + // Repo names that differ only in where the slash falls must not collide: + // "a-b/c" and "a/b-c" joined with a dash are the same string. + sha2 := originRepo(t, filepath.Join(base, "a-b/c")) + sha3 := originRepo(t, filepath.Join(base, "a/b-c")) + one, err := ws.Checkout(ctx, "a-b/c", 1, sha2) + if err != nil { + t.Fatal(err) + } + two, err := ws.Checkout(ctx, "a/b-c", 1, sha3) + if err != nil { + t.Fatal(err) + } + if _, err := os.Stat(one.Dir); err != nil { + t.Errorf("checking out a/b-c destroyed a-b/c's worktree: %v", err) + } + if strings.HasPrefix(two.Dir, one.Dir) || strings.HasPrefix(one.Dir, two.Dir) { + t.Errorf("checkout paths overlap: %q and %q", one.Dir, two.Dir) + } + + // A mirror of a private repository is private source: on a shared host the + // umask default would let any local user read it. + info, err := os.Stat(root) + if err != nil { + t.Fatal(err) + } + if perm := info.Mode().Perm(); perm != 0o700 { + t.Errorf("workspace root mode = %o, want 0700", perm) + } +} + +// Two workers starting on the same repository at once must not clone into one +// directory — the second used to fail with "destination path already exists". +func TestMirrorSurvivesAConcurrentFirstClone(t *testing.T) { + base := t.TempDir() + originRepo(t, filepath.Join(base, "owner/thing")) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + + errs := make(chan error, 4) + for i := 0; i < 4; i++ { + go func() { + _, err := ws.Mirror(context.Background(), "owner/thing") + errs <- err + }() + } + for i := 0; i < 4; i++ { + if err := <-errs; err != nil { + t.Errorf("concurrent first clone: %v", err) + } + } +} + +// The token must never reach argv: a process listing, a log line and this +// package's own error strings would all carry it. +func TestGitTokenTravelsInTheEnvironment(t *testing.T) { + ws := Workspace{Root: t.TempDir(), Token: "ghp_secret_value"} + _, err := ws.git(context.Background(), t.TempDir(), "rev-parse", "HEAD") + if err == nil { + t.Fatal("expected an error outside a repository") + } + if strings.Contains(err.Error(), "ghp_secret_value") { + t.Errorf("the token leaked into an error message: %v", err) + } +} From 9bff9ecdf24bffc98a2834bb791094855e748bd2 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 19:44:40 +0200 Subject: [PATCH 03/17] 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. --- internal/crq/workspace.go | 81 +++++++++++++++++++++++++--------- internal/crq/workspace_test.go | 52 ++++++++++++++++++++++ internal/gh/github.go | 5 +++ 3 files changed, 118 insertions(+), 20 deletions(-) diff --git a/internal/crq/workspace.go b/internal/crq/workspace.go index 12fcc22..00f453c 100644 --- a/internal/crq/workspace.go +++ b/internal/crq/workspace.go @@ -9,6 +9,9 @@ import ( "path/filepath" "strconv" "strings" + "time" + + ghapi "github.com/kristofferR/coderabbit-queue/internal/gh" ) // Workspace is crq's own place on disk for a repository. @@ -66,10 +69,17 @@ func (w Workspace) git(ctx context.Context, dir string, args ...string) (string, } func (w Workspace) root() (string, error) { - if strings.TrimSpace(w.Root) != "" { - return w.Root, nil + root := strings.TrimSpace(w.Root) + if root == "" { + var err error + if root, err = DefaultWorkspaceRoot(); err != nil { + return "", err + } } - return DefaultWorkspaceRoot() + // Absolute, always: `git worktree add` runs inside the mirror, so a relative + // path would create the worktree under the mirror while the path handed back + // points at a directory that does not exist. + return filepath.Abs(root) } // mirrorPath is where repo's bare mirror lives. The owner and name are path @@ -121,10 +131,20 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { } } if _, err := os.Stat(filepath.Join(path, "HEAD")); err == nil { - if _, err := w.git(ctx, path, "fetch", "--prune", "origin"); err != nil { - return "", fmt.Errorf("fetching %s: %w", repo, err) + // Two workers fetching one mirror race on git's ref locks, and the loser + // reports "cannot lock ref" even though the winner has just made the + // mirror current. Retry briefly, then accept the mirror as it stands + // rather than failing a dispatch over a fetch somebody else completed. + var ferr error + for attempt := 0; attempt < 3; attempt++ { + if _, ferr = w.git(ctx, path, "fetch", "--prune", "origin"); ferr == nil { + return path, nil + } + if err := sleepCtx(ctx, time.Duration(attempt+1)*200*time.Millisecond); err != nil { + return "", err + } } - return path, nil + return path, fmt.Errorf("fetching %s: %w", repo, ferr) } else if !errors.Is(err, os.ErrNotExist) { return "", err } @@ -192,10 +212,11 @@ func (w Workspace) Checkout(ctx context.Context, repo string, pr int, sha string if err != nil { return Checkout{}, err } - // Anything already here belongs to an earlier checkout of this PR, which is - // finished or dead either way. Clearing it keeps the generation directories - // from accumulating without letting a live handle delete a newer sibling. - if err := w.clearWork(ctx, mirror, prDir); err != nil { + // Old generations are pruned by AGE, not by being there: another worker may + // be building in one right now, and force-removing it would pull the ground + // out from under a live session. Each handle removes its own on the way out; + // this only collects what a killed process left behind. + if err := w.pruneStaleWork(ctx, mirror, prDir); err != nil { return Checkout{}, err } token := randomToken() @@ -224,8 +245,14 @@ func (w Workspace) workPath(repo string, pr int) (string, error) { return filepath.Join(root, "work", owner, name, strconv.Itoa(pr)), nil } -// clearWork removes every checkout under prDir and deregisters it. -func (w Workspace) clearWork(ctx context.Context, mirror, prDir string) error { +// staleWorkAge is how long an abandoned checkout survives. Longer than any fix +// session should take, short enough that a killed watcher does not leave the +// disk filling up. +const staleWorkAge = 12 * time.Hour + +// pruneStaleWork removes checkouts under prDir that nothing has touched for +// staleWorkAge, leaving live ones alone. +func (w Workspace) pruneStaleWork(ctx context.Context, mirror, prDir string) error { entries, err := os.ReadDir(prDir) if errors.Is(err, os.ErrNotExist) { return nil @@ -234,6 +261,10 @@ func (w Workspace) clearWork(ctx context.Context, mirror, prDir string) error { return err } for _, entry := range entries { + info, err := entry.Info() + if err != nil || time.Since(info.ModTime()) < staleWorkAge { + continue + } if err := w.removeWorktree(ctx, mirror, filepath.Join(prDir, entry.Name())); err != nil { return err } @@ -301,17 +332,27 @@ func gitEnv(ctx context.Context, dir string, env []string, args ...string) (stri // workspace is crq's workspace as this configuration describes it: the root from // the config file (not just the process environment) and the token crq already // resolved for the API, so a daemon with GITHUB_TOKEN alone can still clone. -func (s *Service) workspace() Workspace { - return Workspace{Root: s.cfg.WorkspaceRoot, Token: gitToken()} +func (s *Service) workspace(ctx context.Context) Workspace { + return Workspace{Root: s.cfg.WorkspaceRoot, Token: gitToken(ctx)} } // gitToken is the token to authenticate git with, or "" to leave git to the // host's own credential helper. -func gitToken() string { - for _, name := range []string{"GITHUB_TOKEN", "GH_TOKEN"} { - if value := strings.TrimSpace(os.Getenv(name)); value != "" { - return value - } +// +// It uses the SAME resolution the API client does, including `gh auth token`. +// Reading only the environment meant the documented `gh auth login` setup — API +// calls working fine — still produced unauthenticated clones, so every private +// checkout failed at dispatch's first real act. +func gitToken(ctx context.Context) string { return ghapi.LookupToken(ctx) } + +// sleepCtx waits, or returns early when the context ends. +func sleepCtx(ctx context.Context, d time.Duration) error { + timer := time.NewTimer(d) + defer timer.Stop() + select { + case <-ctx.Done(): + return ctx.Err() + case <-timer.C: + return nil } - return "" } diff --git a/internal/crq/workspace_test.go b/internal/crq/workspace_test.go index 6a14e58..7626487 100644 --- a/internal/crq/workspace_test.go +++ b/internal/crq/workspace_test.go @@ -212,3 +212,55 @@ func TestGitTokenTravelsInTheEnvironment(t *testing.T) { t.Errorf("the token leaked into an error message: %v", err) } } + +// A relative root would make `git worktree add` — which runs inside the mirror — +// create the worktree under the mirror, while the path handed back points at a +// directory that does not exist. +func TestWorkspaceResolvesARelativeRoot(t *testing.T) { + base := t.TempDir() + sha := originRepo(t, filepath.Join(base, "owner/thing")) + t.Setenv("CRQ_REMOTE_BASE", base) + + // Run from a scratch directory so a relative root has somewhere to land. + scratch := t.TempDir() + prev, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + if err := os.Chdir(scratch); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chdir(prev) }) + + co, err := Workspace{Root: "relative-cache"}.Checkout(context.Background(), "owner/thing", 5, sha) + if err != nil { + t.Fatal(err) + } + if !filepath.IsAbs(co.Dir) { + t.Errorf("checkout dir %q is relative", co.Dir) + } + if _, err := os.Stat(filepath.Join(co.Dir, "README.md")); err != nil { + t.Errorf("the checkout is not where it says it is: %v", err) + } +} + +// Another worker may be building in an earlier generation right now. Clearing +// the directory eagerly pulled the ground out from under a live session. +func TestCheckoutLeavesALiveSiblingAlone(t *testing.T) { + base := t.TempDir() + sha := originRepo(t, filepath.Join(base, "owner/thing")) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + + first, err := ws.Checkout(ctx, "owner/thing", 6, sha) + if err != nil { + t.Fatal(err) + } + if _, err := ws.Checkout(ctx, "owner/thing", 6, sha); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(first.Dir, "README.md")); err != nil { + t.Errorf("a live checkout was removed by the next one: %v", err) + } +} diff --git a/internal/gh/github.go b/internal/gh/github.go index 0b81662..d4e8261 100644 --- a/internal/gh/github.go +++ b/internal/gh/github.go @@ -167,6 +167,11 @@ func (g *GitHub) cacheGET(url string, resp *http.Response) (*http.Response, erro return resp, nil } +// LookupToken resolves a GitHub token from the environment or the gh CLI, for +// callers outside this package that need the same credential — git, which does +// not read GITHUB_TOKEN or gh's store by itself. +func LookupToken(ctx context.Context) string { return lookupToken(ctx) } + // lookupToken resolves a GitHub token from the environment or the gh CLI. gh can // hand back a freshly-rotated OAuth token, which is why send re-runs this on a 401. func lookupToken(ctx context.Context) string { From 16c60043c0b5a4aa9cb18e9f87bf443d35109bee Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 21:34:45 +0200 Subject: [PATCH 04/17] Stop the mirror deleting the branches sessions create MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/crq/workspace.go | 41 ++++++++++++++++++++++---- internal/crq/workspace_test.go | 54 ++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 5 deletions(-) diff --git a/internal/crq/workspace.go b/internal/crq/workspace.go index 00f453c..685250b 100644 --- a/internal/crq/workspace.go +++ b/internal/crq/workspace.go @@ -144,6 +144,12 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { return "", err } } + // Still contended. The mirror exists and another worker is updating it, + // so returning an error here fails a dispatch over somebody else's + // success — the case concurrent dispatch is supposed to survive. + if _, statErr := os.Stat(filepath.Join(path, "HEAD")); statErr == nil { + return path, nil + } return path, fmt.Errorf("fetching %s: %w", repo, ferr) } else if !errors.Is(err, os.ErrNotExist) { return "", err @@ -160,9 +166,16 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { } defer os.RemoveAll(staging) pending := filepath.Join(staging, "mirror.git") - if _, err := w.git(ctx, "", "clone", "--mirror", w.remoteURL(repo), pending); err != nil { + // --bare, not --mirror. A mirror's refspec is +refs/*:refs/*, so `fetch + // --prune` reaches into refs/heads and deletes any branch a fix session + // created in a worktree — the documented way to make changes. Remote refs + // live under refs/remotes/origin/*, leaving refs/heads to the sessions. + if _, err := w.git(ctx, "", "clone", "--bare", w.remoteURL(repo), pending); err != nil { return "", fmt.Errorf("cloning %s: %w", repo, err) } + if _, err := w.git(ctx, pending, "config", "remote.origin.fetch", "+refs/heads/*:refs/remotes/origin/*"); err != nil { + return "", fmt.Errorf("configuring %s: %w", repo, err) + } if err := os.Rename(pending, path); err != nil { if _, statErr := os.Stat(filepath.Join(path, "HEAD")); statErr == nil { return path, nil // somebody else won the race; their mirror is as good @@ -224,7 +237,7 @@ func (w Workspace) Checkout(ctx context.Context, repo string, pr int, sha string if err := os.MkdirAll(prDir, 0o700); err != nil { return Checkout{}, err } - if _, err := gitDir(ctx, mirror, "worktree", "add", "--detach", dir, sha); err != nil { + if _, err := w.git(ctx, mirror, "worktree", "add", "--detach", dir, sha); err != nil { return Checkout{}, fmt.Errorf("checking out %s@%s: %w", repo, shortSHA(sha), err) } return Checkout{Dir: dir, Repo: NormalizeRepo(repo), PR: pr, mirror: mirror, token: token}, nil @@ -261,17 +274,35 @@ func (w Workspace) pruneStaleWork(ctx context.Context, mirror, prDir string) err return err } for _, entry := range entries { - info, err := entry.Info() - if err != nil || time.Since(info.ModTime()) < staleWorkAge { + dir := filepath.Join(prDir, entry.Name()) + // The NEWEST file in the checkout, not the directory's own timestamp: + // editing a file or running a build inside leaves the root's mtime + // untouched, so a busy session would read as abandoned. + if touched := newestModTime(dir); time.Since(touched) < staleWorkAge { continue } - if err := w.removeWorktree(ctx, mirror, filepath.Join(prDir, entry.Name())); err != nil { + if err := w.removeWorktree(ctx, mirror, dir); err != nil { return err } } return nil } +// newestModTime is the most recent modification anywhere under dir. +func newestModTime(dir string) time.Time { + newest := time.Time{} + _ = filepath.WalkDir(dir, func(_ string, d os.DirEntry, err error) error { + if err != nil { + return nil + } + if info, ierr := d.Info(); ierr == nil && info.ModTime().After(newest) { + newest = info.ModTime() + } + return nil + }) + return newest +} + // Remove deletes the worktree. Safe to call on an already-removed one. func (c Checkout) Remove(ctx context.Context) error { if c.mirror == "" || c.Dir == "" || filepath.Base(c.Dir) != c.token { diff --git a/internal/crq/workspace_test.go b/internal/crq/workspace_test.go index 7626487..2d98598 100644 --- a/internal/crq/workspace_test.go +++ b/internal/crq/workspace_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "strings" "testing" + "time" ) // originRepo builds a real repository on disk to clone from, so these tests @@ -264,3 +265,56 @@ func TestCheckoutLeavesALiveSiblingAlone(t *testing.T) { t.Errorf("a live checkout was removed by the next one: %v", err) } } + +// A fix session's documented way to make changes is a branch in its checkout. +// A --mirror clone's refspec is +refs/*:refs/*, so the next `fetch --prune` +// reached into refs/heads and deleted that branch — destroying the session's work +// between one dispatch and the next. +func TestFetchDoesNotDeleteABranchAWorktreeCreated(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + sha := originRepo(t, filepath.Join(base, repo)) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + + co, err := ws.Checkout(ctx, repo, 3, sha) + if err != nil { + t.Fatal(err) + } + if _, err := co.Git(ctx, "checkout", "-b", "crq/fix-3"); err != nil { + t.Fatal(err) + } + + // Another dispatch fetches the shared mirror. + if _, err := ws.Mirror(ctx, repo); err != nil { + t.Fatal(err) + } + if branch, err := co.Git(ctx, "rev-parse", "--abbrev-ref", "HEAD"); err != nil || branch != "crq/fix-3" { + t.Fatalf("branch = %q err=%v, want the session's branch to survive a fetch", branch, err) + } +} + +// Pruning read the checkout directory's own mtime, which editing files inside +// does not update — so a busy session read as abandoned and was deleted. +func TestPruningMeasuresTheNewestFileNotTheDirectory(t *testing.T) { + dir := t.TempDir() + old := time.Now().Add(-48 * time.Hour) + nested := filepath.Join(dir, "sub") + if err := os.MkdirAll(nested, 0o755); err != nil { + t.Fatal(err) + } + file := filepath.Join(nested, "edited.go") + if err := os.WriteFile(file, []byte("package main"), 0o644); err != nil { + t.Fatal(err) + } + // The directories look ancient; the file inside was just written. + for _, d := range []string{dir, nested} { + if err := os.Chtimes(d, old, old); err != nil { + t.Fatal(err) + } + } + if since := time.Since(newestModTime(dir)); since > time.Minute { + t.Errorf("newest modification read as %s ago; a live session would be pruned", since) + } +} From 170892387b912ace5e6ad4c75312ebe8814f9251 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 01:27:12 +0200 Subject: [PATCH 05/17] Migrate a mirror's refspec instead of only setting it at clone time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/crq/workspace.go | 14 ++++++++++++- internal/crq/workspace_test.go | 38 ++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/internal/crq/workspace.go b/internal/crq/workspace.go index 685250b..70a0b2f 100644 --- a/internal/crq/workspace.go +++ b/internal/crq/workspace.go @@ -131,6 +131,13 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { } } if _, err := os.Stat(filepath.Join(path, "HEAD")); err == nil { + // Enforce the refspec on EVERY call, not only at clone time. A mirror + // created before this rule still fetches +refs/*:refs/*, and one branch + // created in a worktree then wedges every future fetch for the whole + // repository with "refusing to fetch into branch ... checked out at". + if _, cerr := w.git(ctx, path, "config", "remote.origin.fetch", originRefspec); cerr != nil { + return "", fmt.Errorf("configuring %s: %w", repo, cerr) + } // Two workers fetching one mirror race on git's ref locks, and the loser // reports "cannot lock ref" even though the winner has just made the // mirror current. Retry briefly, then accept the mirror as it stands @@ -173,7 +180,7 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { if _, err := w.git(ctx, "", "clone", "--bare", w.remoteURL(repo), pending); err != nil { return "", fmt.Errorf("cloning %s: %w", repo, err) } - if _, err := w.git(ctx, pending, "config", "remote.origin.fetch", "+refs/heads/*:refs/remotes/origin/*"); err != nil { + if _, err := w.git(ctx, pending, "config", "remote.origin.fetch", originRefspec); err != nil { return "", fmt.Errorf("configuring %s: %w", repo, err) } if err := os.Rename(pending, path); err != nil { @@ -196,6 +203,11 @@ func (w Workspace) remoteURL(repo string) string { return strings.TrimRight(base, "/") + "/" + NormalizeRepo(repo) } +// originRefspec keeps fetched refs out of refs/heads, which belongs to the +// worktrees: a session's branch there must survive a fetch, and a branch checked +// out in a worktree makes any fetch that would update it fail outright. +const originRefspec = "+refs/heads/*:refs/remotes/origin/*" + // Checkout is a worktree at one commit, and the directory git commands for that // PR run in. type Checkout struct { diff --git a/internal/crq/workspace_test.go b/internal/crq/workspace_test.go index 2d98598..f25b91f 100644 --- a/internal/crq/workspace_test.go +++ b/internal/crq/workspace_test.go @@ -318,3 +318,41 @@ func TestPruningMeasuresTheNewestFileNotTheDirectory(t *testing.T) { t.Errorf("newest modification read as %s ago; a live session would be pruned", since) } } + +// A mirror created before the refspec rule still fetches +refs/*:refs/*. One +// branch created in a worktree then wedges every future fetch for the whole +// repository — "refusing to fetch into branch ... checked out at" — which is how +// a single fix session stopped every dispatch for hours. +func TestMirrorMigratesAnOldRefspec(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + sha := originRepo(t, filepath.Join(base, repo)) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + + mirror, err := ws.Mirror(ctx, repo) + if err != nil { + t.Fatal(err) + } + // Put it back the way an older crq left it. + if _, err := gitDir(ctx, mirror, "config", "remote.origin.fetch", "+refs/*:refs/*"); err != nil { + t.Fatal(err) + } + co, err := ws.Checkout(ctx, repo, 4, sha) + if err != nil { + t.Fatal(err) + } + if _, err := co.Git(ctx, "checkout", "-b", "session-branch"); err != nil { + t.Fatal(err) + } + + // The next dispatch of ANY pr in this repository fetches the same mirror. + if _, err := ws.Mirror(ctx, repo); err != nil { + t.Fatalf("a branch in one worktree wedged the whole repository: %v", err) + } + got, err := gitDir(ctx, mirror, "config", "--get", "remote.origin.fetch") + if err != nil || got != originRefspec { + t.Errorf("refspec = %q err=%v, want it migrated to %q", got, err, originRefspec) + } +} From 67408ca874a4fab56b4b765dcb34ecf824086e45 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 04:20:17 +0200 Subject: [PATCH 06/17] Leave refs/heads to the sessions, and stop swallowing a failed fetch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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//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. --- internal/crq/workspace.go | 139 +++++++++++++++++----- internal/crq/workspace_test.go | 205 +++++++++++++++++++++++++++++++++ 2 files changed, 312 insertions(+), 32 deletions(-) diff --git a/internal/crq/workspace.go b/internal/crq/workspace.go index 70a0b2f..ae41add 100644 --- a/internal/crq/workspace.go +++ b/internal/crq/workspace.go @@ -138,6 +138,12 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { if _, cerr := w.git(ctx, path, "config", "remote.origin.fetch", originRefspec); cerr != nil { return "", fmt.Errorf("configuring %s: %w", repo, cerr) } + // A mirror cloned with --mirror also carries remote.origin.mirror=true, + // which the refspec does not clear: a plain `git push` from a session's + // worktree would then mirror the whole local ref namespace, publishing + // internal refs and deleting remote branches this repository has never + // heard of. Not being set is the normal case, and git calls that an error. + _, _ = w.git(ctx, path, "config", "--unset-all", "remote.origin.mirror") // Two workers fetching one mirror race on git's ref locks, and the loser // reports "cannot lock ref" even though the winner has just made the // mirror current. Retry briefly, then accept the mirror as it stands @@ -147,17 +153,21 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { if _, ferr = w.git(ctx, path, "fetch", "--prune", "origin"); ferr == nil { return path, nil } + // Only contention is somebody else's success. Expired credentials, an + // unreachable remote or a corrupt mirror are this caller's problem, and + // swallowing them hands back stale refs — which surfaces later as an + // unreadable commit rather than the fetch error that explains it. + if !isRefLockContention(ferr) { + return "", fmt.Errorf("fetching %s: %w", repo, ferr) + } if err := sleepCtx(ctx, time.Duration(attempt+1)*200*time.Millisecond); err != nil { return "", err } } - // Still contended. The mirror exists and another worker is updating it, - // so returning an error here fails a dispatch over somebody else's - // success — the case concurrent dispatch is supposed to survive. - if _, statErr := os.Stat(filepath.Join(path, "HEAD")); statErr == nil { - return path, nil - } - return path, fmt.Errorf("fetching %s: %w", repo, ferr) + // Still contended. Another worker is updating the mirror right now, so + // returning an error here fails a dispatch over somebody else's success — + // the case concurrent dispatch is supposed to survive. + return path, nil } else if !errors.Is(err, os.ErrNotExist) { return "", err } @@ -173,16 +183,24 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { } defer os.RemoveAll(staging) pending := filepath.Join(staging, "mirror.git") - // --bare, not --mirror. A mirror's refspec is +refs/*:refs/*, so `fetch - // --prune` reaches into refs/heads and deletes any branch a fix session - // created in a worktree — the documented way to make changes. Remote refs - // live under refs/remotes/origin/*, leaving refs/heads to the sessions. - if _, err := w.git(ctx, "", "clone", "--bare", w.remoteURL(repo), pending); err != nil { - return "", fmt.Errorf("cloning %s: %w", repo, err) + // Init and fetch, not `clone --bare`: a bare clone copies the remote's branch + // heads straight into refs/heads, and the refspec set afterwards only governs + // later fetches. refs/heads belongs to the sessions — a branch already sitting + // there is one a session cannot create, and a stale commit it could land on. + // Fetching into an empty repository leaves that namespace to them, and never + // sets the remote.origin.mirror that `clone --mirror` did. + if _, err := w.git(ctx, "", "init", "--bare", "--quiet", pending); err != nil { + return "", fmt.Errorf("creating mirror of %s: %w", repo, err) + } + if _, err := w.git(ctx, pending, "remote", "add", "origin", w.remoteURL(repo)); err != nil { + return "", fmt.Errorf("configuring %s: %w", repo, err) } if _, err := w.git(ctx, pending, "config", "remote.origin.fetch", originRefspec); err != nil { return "", fmt.Errorf("configuring %s: %w", repo, err) } + if _, err := w.git(ctx, pending, "fetch", "--prune", "origin"); err != nil { + return "", fmt.Errorf("cloning %s: %w", repo, err) + } if err := os.Rename(pending, path); err != nil { if _, statErr := os.Stat(filepath.Join(path, "HEAD")); statErr == nil { return path, nil // somebody else won the race; their mirror is as good @@ -208,6 +226,24 @@ func (w Workspace) remoteURL(repo string) string { // out in a worktree makes any fetch that would update it fail outright. const originRefspec = "+refs/heads/*:refs/remotes/origin/*" +// isRefLockContention reports whether a fetch failed because another process +// held the locks — the one failure that is somebody else's success, and so the +// only one worth retrying and then ignoring. Everything else (a bad token, an +// unreachable remote, a broken mirror) has to reach the caller. +func isRefLockContention(err error) bool { + if err == nil { + return false + } + msg := strings.ToLower(err.Error()) + switch { + case strings.Contains(msg, "cannot lock ref"), + strings.Contains(msg, "another git process"), + strings.Contains(msg, ".lock") && strings.Contains(msg, "file exists"): + return true + } + return false +} + // Checkout is a worktree at one commit, and the directory git commands for that // PR run in. type Checkout struct { @@ -215,6 +251,11 @@ type Checkout struct { Repo string PR int mirror string + // ws is the workspace that made this checkout, kept so the git commands run + // inside it authenticate the same way the clone did. A session's push is a + // network command like any other, and a daemon holding only GITHUB_TOKEN has + // no credential helper to fall back on. + ws Workspace // token makes this handle's directory its own. Two checkouts of one PR // otherwise share a path, and a deferred Remove on the older handle deletes // the newer one's worktree out from under it. @@ -241,18 +282,40 @@ func (w Workspace) Checkout(ctx context.Context, repo string, pr int, sha string // be building in one right now, and force-removing it would pull the ground // out from under a live session. Each handle removes its own on the way out; // this only collects what a killed process left behind. - if err := w.pruneStaleWork(ctx, mirror, prDir); err != nil { + if err := pruneStaleWork(ctx, mirror, filepath.Dir(prDir)); err != nil { return Checkout{}, err } + w.fetchPullRef(ctx, mirror, pr, sha) token := randomToken() dir := filepath.Join(prDir, token) if err := os.MkdirAll(prDir, 0o700); err != nil { return Checkout{}, err } - if _, err := w.git(ctx, mirror, "worktree", "add", "--detach", dir, sha); err != nil { + // `--` before the positional arguments: a commit-ish beginning with a dash + // would otherwise be read as an option, which is the shape behind a long line + // of clone-and-checkout CVEs. + if _, err := w.git(ctx, mirror, "worktree", "add", "--detach", "--", dir, sha); err != nil { return Checkout{}, fmt.Errorf("checking out %s@%s: %w", repo, shortSHA(sha), err) } - return Checkout{Dir: dir, Repo: NormalizeRepo(repo), PR: pr, mirror: mirror, token: token}, nil + return Checkout{Dir: dir, Repo: NormalizeRepo(repo), PR: pr, mirror: mirror, ws: w, token: token}, nil +} + +// fetchPullRef fetches refs/pull//head when sha is not in the mirror yet. +// +// A PR opened from a fork has its head on no branch of the base repository, so +// the mirror's refspec never brings it down and the worktree would fail at an +// unknown commit. GitHub publishes it as refs/pull//head. Best effort on +// purpose: the checkout that follows is the real check, and its error names the +// commit that could not be found. +func (w Workspace) fetchPullRef(ctx context.Context, mirror string, pr int, sha string) { + if pr <= 0 { + return + } + if _, err := gitDir(ctx, mirror, "cat-file", "-e", sha+"^{commit}"); err == nil { + return + } + ref := fmt.Sprintf("+refs/pull/%d/head:refs/remotes/origin/pull/%d", pr, pr) + _, _ = w.git(ctx, mirror, "fetch", "origin", ref) } // workPath is the directory holding this PR's checkouts. Owner and name stay @@ -275,26 +338,37 @@ func (w Workspace) workPath(repo string, pr int) (string, error) { // disk filling up. const staleWorkAge = 12 * time.Hour -// pruneStaleWork removes checkouts under prDir that nothing has touched for -// staleWorkAge, leaving live ones alone. -func (w Workspace) pruneStaleWork(ctx context.Context, mirror, prDir string) error { - entries, err := os.ReadDir(prDir) +// pruneStaleWork removes checkouts anywhere under repoDir that nothing has +// touched for staleWorkAge, leaving live ones alone. +// +// Every PR of the repository, not only the one being checked out: a process +// killed mid-dispatch leaves a generation behind, and if that PR is then merged, +// closed or simply never dispatched again, nothing would ever come back to +// collect it. Sweeping the repository means any dispatch of any PR does. +func pruneStaleWork(ctx context.Context, mirror, repoDir string) error { + prDirs, err := os.ReadDir(repoDir) if errors.Is(err, os.ErrNotExist) { return nil } if err != nil { return err } - for _, entry := range entries { - dir := filepath.Join(prDir, entry.Name()) - // The NEWEST file in the checkout, not the directory's own timestamp: - // editing a file or running a build inside leaves the root's mtime - // untouched, so a busy session would read as abandoned. - if touched := newestModTime(dir); time.Since(touched) < staleWorkAge { + for _, prDir := range prDirs { + entries, err := os.ReadDir(filepath.Join(repoDir, prDir.Name())) + if err != nil { continue } - if err := w.removeWorktree(ctx, mirror, dir); err != nil { - return err + for _, entry := range entries { + dir := filepath.Join(repoDir, prDir.Name(), entry.Name()) + // The NEWEST file in the checkout, not the directory's own timestamp: + // editing a file or running a build inside leaves the root's mtime + // untouched, so a busy session would read as abandoned. + if touched := newestModTime(dir); time.Since(touched) < staleWorkAge { + continue + } + if err := removeWorktree(ctx, mirror, dir); err != nil { + return err + } } } return nil @@ -322,10 +396,10 @@ func (c Checkout) Remove(ctx context.Context) error { // it would delete a worktree somebody else is using. return nil } - return Workspace{}.removeWorktree(ctx, c.mirror, c.Dir) + return removeWorktree(ctx, c.mirror, c.Dir) } -func (w Workspace) removeWorktree(ctx context.Context, mirror, dir string) error { +func removeWorktree(ctx context.Context, mirror, dir string) error { if _, err := os.Stat(dir); errors.Is(err, os.ErrNotExist) { // Still prune: git may hold a record of a directory somebody deleted. _, _ = gitDir(ctx, mirror, "worktree", "prune") @@ -341,9 +415,10 @@ func (w Workspace) removeWorktree(ctx context.Context, mirror, dir string) error return nil } -// Git runs a git command inside this checkout. +// Git runs a git command inside this checkout, with the same credentials the +// mirror was cloned with — a session's push is a network command too. func (c Checkout) Git(ctx context.Context, args ...string) (string, error) { - return gitDir(ctx, c.Dir, args...) + return c.ws.git(ctx, c.Dir, args...) } // gitDir runs git in dir ("" means the process's own directory) and returns its diff --git a/internal/crq/workspace_test.go b/internal/crq/workspace_test.go index f25b91f..85e4016 100644 --- a/internal/crq/workspace_test.go +++ b/internal/crq/workspace_test.go @@ -98,6 +98,22 @@ func TestWorkspaceChecksOutAHeadWithoutACheckout(t *testing.T) { } } +// The daemon's workspace has to come from the config file, not the process +// environment, and its token from the same resolution the API client uses: +// reading only the environment meant a documented `gh auth login` setup made API +// calls work while every private clone came back unauthenticated. +func TestServiceWorkspaceUsesTheConfiguredRootAndToken(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "ghp_from_the_environment") + s := &Service{cfg: Config{WorkspaceRoot: "/tmp/crq-configured-root"}} + ws := s.workspace(context.Background()) + if ws.Root != "/tmp/crq-configured-root" { + t.Errorf("workspace root = %q, want the configured one", ws.Root) + } + if ws.Token != "ghp_from_the_environment" { + t.Errorf("workspace token = %q, want the one the API client resolves", ws.Token) + } +} + // A repo name is turned into path segments, so anything that could climb out of // the workspace root has to be refused rather than joined. func TestWorkspaceRefusesRepoNamesThatEscape(t *testing.T) { @@ -319,6 +335,195 @@ func TestPruningMeasuresTheNewestFileNotTheDirectory(t *testing.T) { } } +// `clone --bare` copies the remote's branch heads straight into refs/heads, and +// a refspec set afterwards only governs later fetches. A session could then not +// create its branch — the name is taken — or would land on the commit the clone +// froze there. +func TestANewMirrorLeavesRefsHeadsToTheSessions(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + sha := originRepo(t, filepath.Join(base, repo)) + if _, err := gitDir(context.Background(), filepath.Join(base, repo), "branch", "feature"); err != nil { + t.Fatal(err) + } + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + + mirror, err := ws.Mirror(ctx, repo) + if err != nil { + t.Fatal(err) + } + if heads, err := gitDir(ctx, mirror, "for-each-ref", "refs/heads"); err != nil || heads != "" { + t.Errorf("refs/heads = %q err=%v, want it empty and left to the sessions", heads, err) + } + if remotes, err := gitDir(ctx, mirror, "for-each-ref", "--format=%(refname)", "refs/remotes/origin"); err != nil || !strings.Contains(remotes, "refs/remotes/origin/feature") { + t.Errorf("remote refs = %q err=%v, want the branches fetched under origin", remotes, err) + } + + // The name a remote branch occupies is therefore free for a session. + co, err := ws.Checkout(ctx, repo, 8, sha) + if err != nil { + t.Fatal(err) + } + if _, err := co.Git(ctx, "checkout", "-b", "feature"); err != nil { + t.Errorf("a session could not create its branch: %v", err) + } +} + +// A mirror cloned by an older crq also has remote.origin.mirror=true, which the +// refspec does not clear: a plain push from a session's worktree would mirror +// every local ref, publishing internal refs and deleting remote branches. +func TestMirrorMigrationClearsThePushMirrorFlag(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + originRepo(t, filepath.Join(base, repo)) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + + mirror, err := ws.Mirror(ctx, repo) + if err != nil { + t.Fatal(err) + } + if _, err := gitDir(ctx, mirror, "config", "remote.origin.mirror", "true"); err != nil { + t.Fatal(err) + } + if _, err := ws.Mirror(ctx, repo); err != nil { + t.Fatal(err) + } + if got, err := gitDir(ctx, mirror, "config", "--get", "remote.origin.mirror"); err == nil { + t.Errorf("remote.origin.mirror = %q, want it unset", got) + } +} + +// The mirror's HEAD exists because the mirror exists, so testing for it after a +// failed fetch declared every failure contention: expired credentials and an +// unreachable remote came back as success with stale refs, and the caller met +// them later as an unreadable commit instead. +func TestMirrorReportsAFetchFailureThatIsNotContention(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + originRepo(t, filepath.Join(base, repo)) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + + if _, err := ws.Mirror(ctx, repo); err != nil { + t.Fatal(err) + } + // The remote goes away, the way an expired token or a network outage does. + if err := os.RemoveAll(filepath.Join(base, repo)); err != nil { + t.Fatal(err) + } + if _, err := ws.Mirror(ctx, repo); err == nil { + t.Error("a failed fetch was reported as a current mirror") + } +} + +// A PR opened from a fork has its head on no branch of the base repository, so +// the mirror's refspec never brings it down; GitHub publishes it as +// refs/pull//head. +func TestCheckoutFetchesAForkHeadFromThePullRef(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + origin := filepath.Join(base, repo) + originRepo(t, origin) + ctx := context.Background() + run := func(args ...string) string { + t.Helper() + out, err := gitDir(ctx, origin, args...) + if err != nil { + t.Fatalf("git %s: %v", strings.Join(args, " "), err) + } + return out + } + // A commit reachable from the pull ref alone, exactly like a fork's head. + run("checkout", "-q", "-b", "from-a-fork") + if err := os.WriteFile(filepath.Join(origin, "fork.txt"), []byte("two\n"), 0o644); err != nil { + t.Fatal(err) + } + run("add", "fork.txt") + run("commit", "-m", "fork work") + forkSHA := run("rev-parse", "HEAD") + run("update-ref", "refs/pull/11/head", forkSHA) + run("checkout", "-q", "main") + run("branch", "-qD", "from-a-fork") + + t.Setenv("CRQ_REMOTE_BASE", base) + co, err := Workspace{Root: t.TempDir()}.Checkout(ctx, repo, 11, forkSHA) + if err != nil { + t.Fatalf("checking out a fork head: %v", err) + } + if head, err := co.Git(ctx, "rev-parse", "HEAD"); err != nil || head != forkSHA { + t.Errorf("checked out %q err=%v, want the fork head %q", head, err, forkSHA) + } +} + +// Pruning ran only under the PR being checked out, so a generation left by a +// killed process was collected only if that same PR was dispatched again — never, +// once it was merged or closed. +func TestPruningCollectsAnAbandonedCheckoutOfAnotherPR(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + sha := originRepo(t, filepath.Join(base, repo)) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + + abandoned, err := ws.workPath(repo, 21) + if err != nil { + t.Fatal(err) + } + abandoned = filepath.Join(abandoned, "leftover") + if err := os.MkdirAll(abandoned, 0o700); err != nil { + t.Fatal(err) + } + file := filepath.Join(abandoned, "stale.txt") + if err := os.WriteFile(file, []byte("x"), 0o600); err != nil { + t.Fatal(err) + } + old := time.Now().Add(-2 * staleWorkAge) + for _, p := range []string{file, abandoned} { + if err := os.Chtimes(p, old, old); err != nil { + t.Fatal(err) + } + } + + // A dispatch of a different PR is the only visitor this repository gets. + if _, err := ws.Checkout(ctx, repo, 22, sha); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(abandoned); !os.IsNotExist(err) { + t.Errorf("an abandoned checkout of another PR survived: %v", err) + } +} + +// A daemon holding only GITHUB_TOKEN has no credential helper, so a push from a +// session's checkout could not authenticate even though the clone did. +func TestCheckoutGitCarriesTheWorkspaceCredentials(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + sha := originRepo(t, filepath.Join(base, repo)) + t.Setenv("CRQ_REMOTE_BASE", base) + ctx := context.Background() + + co, err := Workspace{Root: t.TempDir(), Token: "ghp_secret_value"}.Checkout(ctx, repo, 12, sha) + if err != nil { + t.Fatal(err) + } + helper, err := co.Git(ctx, "config", "--get-all", "credential.helper") + if err != nil { + t.Fatal(err) + } + if !strings.Contains(helper, "CRQ_GIT_TOKEN") { + t.Errorf("credential.helper = %q, want the workspace's helper", helper) + } + if strings.Contains(helper, "ghp_secret_value") { + t.Errorf("the token itself reached git's configuration: %q", helper) + } +} + // A mirror created before the refspec rule still fetches +refs/*:refs/*. One // branch created in a worktree then wedges every future fetch for the whole // repository — "refusing to fetch into branch ... checked out at" — which is how From 036c830c907ee264f8758b88f7112e7839ceb13c Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 04:45:22 +0200 Subject: [PATCH 07/17] Answer credentials for GitHub only, and stop pruning live checkouts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/crq/workspace.go | 129 +++++++++++++++++++++++++++++---- internal/crq/workspace_test.go | 81 +++++++++++++++++++++ 2 files changed, 196 insertions(+), 14 deletions(-) diff --git a/internal/crq/workspace.go b/internal/crq/workspace.go index ae41add..6e70472 100644 --- a/internal/crq/workspace.go +++ b/internal/crq/workspace.go @@ -26,10 +26,11 @@ import ( // than re-cloned, and a throwaway worktree per head. Nothing here reads or // writes the process's working directory. // -// Credentials are deliberately not crq's business. The remote is the ordinary -// https URL, so whatever git credential helper the host already uses — `gh auth -// setup-git` writes one — supplies the token. crq never holds a secret it would -// then have to avoid logging. +// Credentials stay as close to the host's own as they can be. The remote is the +// ordinary https URL, so whatever git credential helper the host already uses — +// `gh auth setup-git` writes one — supplies the token. Only a daemon that has no +// such helper makes crq inject one of its own (see credentialHelper), and even +// then the secret lives in the environment rather than in argv or a config file. type Workspace struct { // Root holds the mirrors and worktrees. Empty means the default cache // location (see DefaultWorkspaceRoot). @@ -56,18 +57,32 @@ func DefaultWorkspaceRoot() (string, error) { // git runs a git command for this workspace, injecting credentials when a token // is configured. // -// The token travels in the ENVIRONMENT, never in argv: the helper below is a -// shell snippet that reads it, so a process listing, a log line and this -// package's own error strings all carry the snippet and never the secret. +// The token travels in the ENVIRONMENT, never in argv: the helper is a shell +// snippet that reads it, so a process listing, a log line and this package's own +// error strings all carry the snippet and never the secret. func (w Workspace) git(ctx context.Context, dir string, args ...string) (string, error) { if strings.TrimSpace(w.Token) == "" { return gitDir(ctx, dir, args...) } - const helper = `!f() { test "$1" = get && printf 'username=x-access-token\npassword=%s\n' "$CRQ_GIT_TOKEN"; }; f` - full := append([]string{"-c", "credential.helper=", "-c", "credential.helper=" + helper}, args...) + full := append([]string{"-c", "credential.helper=", "-c", "credential.helper=" + credentialHelper}, args...) return gitEnv(ctx, dir, []string{"CRQ_GIT_TOKEN=" + w.Token}, full...) } +// credentialHelper answers for https://github.com and nothing else. +// +// A git command can be led to a URL that repository CONTENT chose — a submodule, +// an LFS endpoint — and a helper that answers every request would hand the +// account's token to whatever host a pull request pointed it at. git writes the +// request to stdin as key=value lines and appends the operation as an argument, +// so read the protocol and host and stay quiet unless they are ours. The lines +// are drained whatever the operation is, so a store or an erase does not meet a +// closed pipe, and the snippet always exits 0. +// +// github.com literally: the token is a GitHub token, and CRQ_REMOTE_BASE (the +// only other remote crq ever uses) points at a local path in tests, which needs +// no credentials at all. +const credentialHelper = `!f() { p=; h=; while IFS= read -r l; do case "$l" in protocol=*) p=${l#protocol=} ;; host=*) h=${l#host=} ;; esac; done; if test "$1" = get && test "$p" = https && test "$h" = github.com; then printf 'username=x-access-token\npassword=%s\n' "$CRQ_GIT_TOKEN"; fi; }; f` + func (w Workspace) root() (string, error) { root := strings.TrimSpace(w.Root) if root == "" { @@ -135,15 +150,18 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { // created before this rule still fetches +refs/*:refs/*, and one branch // created in a worktree then wedges every future fetch for the whole // repository with "refusing to fetch into branch ... checked out at". - if _, cerr := w.git(ctx, path, "config", "remote.origin.fetch", originRefspec); cerr != nil { + if cerr := w.setConfig(ctx, path, "remote.origin.fetch", originRefspec); cerr != nil { return "", fmt.Errorf("configuring %s: %w", repo, cerr) } // A mirror cloned with --mirror also carries remote.origin.mirror=true, // which the refspec does not clear: a plain `git push` from a session's // worktree would then mirror the whole local ref namespace, publishing // internal refs and deleting remote branches this repository has never - // heard of. Not being set is the normal case, and git calls that an error. - _, _ = w.git(ctx, path, "config", "--unset-all", "remote.origin.mirror") + // heard of. Read before unsetting: not being set is the normal case, and + // an unconditional --unset-all takes config.lock on every single call. + if _, gerr := w.git(ctx, path, "config", "--get", "remote.origin.mirror"); gerr == nil { + _, _ = w.git(ctx, path, "config", "--unset-all", "remote.origin.mirror") + } // Two workers fetching one mirror race on git's ref locks, and the loser // reports "cannot lock ref" even though the winner has just made the // mirror current. Retry briefly, then accept the mirror as it stands @@ -226,6 +244,49 @@ func (w Workspace) remoteURL(repo string) string { // out in a worktree makes any fetch that would update it fail outright. const originRefspec = "+refs/heads/*:refs/remotes/origin/*" +// setConfig writes one config key of the mirror at path, unless it already holds +// value. +// +// git serializes configuration writes through config.lock, so two dispatches of +// the same repository arriving together make one of them fail with "could not +// lock config file" — before the fetch, which is the part written to tolerate +// concurrency. Reading first takes that write out of every call but the one that +// actually migrates something, and that one retries. +func (w Workspace) setConfig(ctx context.Context, path, key, value string) error { + var err error + for attempt := 0; attempt < 3; attempt++ { + if cur, gerr := w.git(ctx, path, "config", "--get", key); gerr == nil && cur == value { + return nil + } + if _, err = w.git(ctx, path, "config", key, value); err == nil { + return nil + } + if !isConfigLockContention(err) { + return err + } + if serr := sleepCtx(ctx, time.Duration(attempt+1)*200*time.Millisecond); serr != nil { + return serr + } + } + // Still contended: the other worker is writing this same migration, so its + // result is the one we wanted. Only its absence is worth an error. + if cur, gerr := w.git(ctx, path, "config", "--get", key); gerr == nil && cur == value { + return nil + } + return err +} + +// isConfigLockContention reports whether a config write lost the race for +// config.lock — git's own serialization, and so somebody else's success. +func isConfigLockContention(err error) bool { + if err == nil { + return false + } + msg := strings.ToLower(err.Error()) + return strings.Contains(msg, "could not lock config file") || + strings.Contains(msg, "another git process") +} + // isRefLockContention reports whether a fetch failed because another process // held the locks — the one failure that is somebody else's success, and so the // only one worth retrying and then ignoring. Everything else (a bad token, an @@ -260,6 +321,9 @@ type Checkout struct { // otherwise share a path, and a deferred Remove on the older handle deletes // the newer one's worktree out from under it. token string + // stop ends the heartbeat that keeps this checkout from being pruned while + // the process holding it is alive. + stop context.CancelFunc } // Checkout creates a worktree of repo at sha, on a detached HEAD. @@ -297,7 +361,12 @@ func (w Workspace) Checkout(ctx context.Context, repo string, pr int, sha string if _, err := w.git(ctx, mirror, "worktree", "add", "--detach", "--", dir, sha); err != nil { return Checkout{}, fmt.Errorf("checking out %s@%s: %w", repo, shortSHA(sha), err) } - return Checkout{Dir: dir, Repo: NormalizeRepo(repo), PR: pr, mirror: mirror, ws: w, token: token}, nil + // Held for as long as the caller's context lives, which is as long as this + // handle is worth anything. A caller whose context ends at once gets what it + // got before: a checkout that ages out of the workspace on its own. + alive, stop := context.WithCancel(ctx) + go keepAlive(alive, dir, heartbeatInterval) + return Checkout{Dir: dir, Repo: NormalizeRepo(repo), PR: pr, mirror: mirror, ws: w, token: token, stop: stop}, nil } // fetchPullRef fetches refs/pull//head when sha is not in the mirror yet. @@ -338,6 +407,34 @@ func (w Workspace) workPath(repo string, pr int) (string, error) { // disk filling up. const staleWorkAge = 12 * time.Hour +// heartbeatInterval is how often a live checkout refreshes itself. Far inside +// staleWorkAge, so a checkout ages out only once nothing has refreshed it for +// several dozen missed beats. +const heartbeatInterval = 15 * time.Minute + +// keepAlive refreshes dir's own timestamp until ctx ends. +// +// Age alone does not mean abandoned: a session can legitimately touch no file +// for hours — holding uncommitted fixes while it waits on a reviewer or on the +// queue — and pruning would then delete a checkout somebody is still using. Age +// SINCE THE LAST BEAT does mean it, because the only thing that stops the beat +// is the process that owns the checkout going away. +// +// The directory's own timestamp, not a marker file: it is what newestModTime +// already reads, and a file inside would turn up in the session's own git +// status and in the diff it is about to commit. +func keepAlive(ctx context.Context, dir string, every time.Duration) { + for { + if err := sleepCtx(ctx, every); err != nil { + return + } + now := time.Now() + if err := os.Chtimes(dir, now, now); err != nil { + return // the checkout is gone; there is nothing left to keep alive + } + } +} + // pruneStaleWork removes checkouts anywhere under repoDir that nothing has // touched for staleWorkAge, leaving live ones alone. // @@ -362,7 +459,8 @@ func pruneStaleWork(ctx context.Context, mirror, repoDir string) error { dir := filepath.Join(repoDir, prDir.Name(), entry.Name()) // The NEWEST file in the checkout, not the directory's own timestamp: // editing a file or running a build inside leaves the root's mtime - // untouched, so a busy session would read as abandoned. + // untouched, so a busy session would read as abandoned. An idle one + // keeps that root fresh itself (keepAlive). if touched := newestModTime(dir); time.Since(touched) < staleWorkAge { continue } @@ -391,6 +489,9 @@ func newestModTime(dir string) time.Time { // Remove deletes the worktree. Safe to call on an already-removed one. func (c Checkout) Remove(ctx context.Context) error { + if c.stop != nil { + c.stop() // this handle is done either way, so stop claiming it is live + } if c.mirror == "" || c.Dir == "" || filepath.Base(c.Dir) != c.token { // Not this handle's directory: a later checkout replaced it, and removing // it would delete a worktree somebody else is using. diff --git a/internal/crq/workspace_test.go b/internal/crq/workspace_test.go index 85e4016..e9826b3 100644 --- a/internal/crq/workspace_test.go +++ b/internal/crq/workspace_test.go @@ -3,6 +3,7 @@ package crq import ( "context" "os" + "os/exec" "path/filepath" "strings" "testing" @@ -561,3 +562,83 @@ func TestMirrorMigratesAnOldRefspec(t *testing.T) { t.Errorf("refspec = %q err=%v, want it migrated to %q", got, err, originRefspec) } } + +// git serializes config writes through config.lock, so an unconditional write on +// every Mirror call made two dispatches of one repository collide with "could +// not lock config file" — before the fetch, which is the part written to survive +// concurrency. A mirror already holding the right value must not write at all. +func TestMirrorDoesNotWriteConfigThatIsAlreadyCurrent(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + originRepo(t, filepath.Join(base, repo)) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + + mirror, err := ws.Mirror(ctx, repo) + if err != nil { + t.Fatal(err) + } + // Somebody else is holding the lock, exactly as a concurrent dispatch would. + if err := os.WriteFile(filepath.Join(mirror, "config.lock"), nil, 0o600); err != nil { + t.Fatal(err) + } + defer os.Remove(filepath.Join(mirror, "config.lock")) + if _, err := ws.Mirror(ctx, repo); err != nil { + t.Errorf("a held config.lock failed a dispatch that had nothing to write: %v", err) + } +} + +// A git command can be led to a URL that repository content chose — a submodule, +// an LFS endpoint — so a helper that answered every request would hand a pull +// request the account's token. +func TestCredentialHelperAnswersOnlyForGitHub(t *testing.T) { + ask := func(t *testing.T, request string) string { + t.Helper() + cmd := exec.Command("sh", "-c", strings.TrimPrefix(credentialHelper, "!")+" get") + cmd.Stdin = strings.NewReader(request) + cmd.Env = append(os.Environ(), "CRQ_GIT_TOKEN=ghp_secret_value") + out, err := cmd.Output() + if err != nil { + t.Fatalf("credential helper failed on %q: %v", request, err) + } + return string(out) + } + if got := ask(t, "protocol=https\nhost=github.com\n\n"); !strings.Contains(got, "password=ghp_secret_value") { + t.Errorf("helper answered github.com with %q, want the token", got) + } + for _, request := range []string{ + "protocol=https\nhost=evil.example\n\n", + "protocol=https\nhost=github.com.evil.example\n\n", + "protocol=http\nhost=github.com\n\n", + } { + if got := ask(t, request); strings.Contains(got, "ghp_secret_value") { + t.Errorf("helper leaked the token to %q: %q", request, got) + } + } +} + +// A session sitting idle — holding uncommitted fixes while it waits on a +// reviewer — touches no file for hours, and pruning by age alone would delete +// the checkout out from under it. A live handle keeps its own timestamp fresh. +func TestALiveCheckoutKeepsItselfFromBeingPruned(t *testing.T) { + dir := t.TempDir() + old := time.Now().Add(-2 * staleWorkAge) + if err := os.Chtimes(dir, old, old); err != nil { + t.Fatal(err) + } + if since := time.Since(newestModTime(dir)); since < staleWorkAge { + t.Fatalf("the checkout reads as %s old, wanted it stale to begin with", since) + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go keepAlive(ctx, dir, time.Millisecond) + for deadline := time.Now().Add(2 * time.Second); time.Now().Before(deadline); { + if time.Since(newestModTime(dir)) < time.Minute { + return + } + time.Sleep(5 * time.Millisecond) + } + t.Error("an idle checkout still read as abandoned while its process was alive") +} From 2ca6e83fa76f68851c84dea4933724da9b9c2c2b Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 05:07:41 +0200 Subject: [PATCH 08/17] Finish the migration of a mirror an older crq made MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/crq/workspace.go | 185 ++++++++++++++++++++++++++------- internal/crq/workspace_test.go | 160 ++++++++++++++++++++++++++++ 2 files changed, 306 insertions(+), 39 deletions(-) diff --git a/internal/crq/workspace.go b/internal/crq/workspace.go index 6e70472..c044e4c 100644 --- a/internal/crq/workspace.go +++ b/internal/crq/workspace.go @@ -146,45 +146,18 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { } } if _, err := os.Stat(filepath.Join(path, "HEAD")); err == nil { - // Enforce the refspec on EVERY call, not only at clone time. A mirror - // created before this rule still fetches +refs/*:refs/*, and one branch - // created in a worktree then wedges every future fetch for the whole - // repository with "refusing to fetch into branch ... checked out at". - if cerr := w.setConfig(ctx, path, "remote.origin.fetch", originRefspec); cerr != nil { + if cerr := w.migrateMirror(ctx, path); cerr != nil { return "", fmt.Errorf("configuring %s: %w", repo, cerr) } - // A mirror cloned with --mirror also carries remote.origin.mirror=true, - // which the refspec does not clear: a plain `git push` from a session's - // worktree would then mirror the whole local ref namespace, publishing - // internal refs and deleting remote branches this repository has never - // heard of. Read before unsetting: not being set is the normal case, and - // an unconditional --unset-all takes config.lock on every single call. - if _, gerr := w.git(ctx, path, "config", "--get", "remote.origin.mirror"); gerr == nil { - _, _ = w.git(ctx, path, "config", "--unset-all", "remote.origin.mirror") + if ferr := w.fetchMirror(ctx, path); ferr != nil { + return "", fmt.Errorf("fetching %s: %w", repo, ferr) } - // Two workers fetching one mirror race on git's ref locks, and the loser - // reports "cannot lock ref" even though the winner has just made the - // mirror current. Retry briefly, then accept the mirror as it stands - // rather than failing a dispatch over a fetch somebody else completed. - var ferr error - for attempt := 0; attempt < 3; attempt++ { - if _, ferr = w.git(ctx, path, "fetch", "--prune", "origin"); ferr == nil { - return path, nil - } - // Only contention is somebody else's success. Expired credentials, an - // unreachable remote or a corrupt mirror are this caller's problem, and - // swallowing them hands back stale refs — which surfaces later as an - // unreadable commit rather than the fetch error that explains it. - if !isRefLockContention(ferr) { - return "", fmt.Errorf("fetching %s: %w", repo, ferr) - } - if err := sleepCtx(ctx, time.Duration(attempt+1)*200*time.Millisecond); err != nil { - return "", err - } + // After the fetch, because it is the fetch that puts the current value of + // every remote branch under refs/remotes/origin — which is what makes the + // copies an old clone left in refs/heads redundant. + if derr := w.dropFetchedHeads(ctx, path); derr != nil { + return "", fmt.Errorf("migrating %s: %w", repo, derr) } - // Still contended. Another worker is updating the mirror right now, so - // returning an error here fails a dispatch over somebody else's success — - // the case concurrent dispatch is supposed to survive. return path, nil } else if !errors.Is(err, os.ErrNotExist) { return "", err @@ -244,8 +217,108 @@ func (w Workspace) remoteURL(repo string) string { // out in a worktree makes any fetch that would update it fail outright. const originRefspec = "+refs/heads/*:refs/remotes/origin/*" +// migrateMirror brings a mirror an older crq left behind up to the rules this +// one depends on, and is a no-op on a mirror that already follows them. +func (w Workspace) migrateMirror(ctx context.Context, path string) error { + // Enforce the refspec on EVERY call, not only at clone time. A mirror + // created before this rule still fetches +refs/*:refs/*, and one branch + // created in a worktree then wedges every future fetch for the whole + // repository with "refusing to fetch into branch ... checked out at". + if err := w.setConfig(ctx, path, "remote.origin.fetch", originRefspec); err != nil { + return err + } + // A mirror cloned with --mirror also carries remote.origin.mirror=true, which + // the refspec does not clear: a plain `git push` from a session's worktree + // would then mirror the whole local ref namespace, publishing internal refs + // and deleting remote branches this repository has never heard of. + return w.unsetConfig(ctx, path, "remote.origin.mirror") +} + +// fetchMirror brings the mirror at path up to date. +func (w Workspace) fetchMirror(ctx context.Context, path string) error { + // Two workers fetching one mirror race on git's ref locks, and the loser + // reports "cannot lock ref" even though the winner has just made the mirror + // current. Retry briefly rather than failing a dispatch over that. + var err error + for attempt := 0; attempt < 3; attempt++ { + if _, err = w.git(ctx, path, "fetch", "--prune", "origin"); err == nil { + return nil + } + // Only contention is somebody else's success. Expired credentials, an + // unreachable remote or a corrupt mirror are this caller's problem, and + // swallowing them hands back stale refs — which surfaces later as an + // unreadable commit rather than the fetch error that explains it. + if !isRefLockContention(err) { + return err + } + if serr := sleepCtx(ctx, time.Duration(attempt+1)*200*time.Millisecond); serr != nil { + return serr + } + } + // Three losses in a row is no longer evidence of somebody else's success: 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. Report the fetch that did not + // happen — the alternative is stale refs presented as current ones. + return err +} + +// dropFetchedHeads removes the branch copies an older `git clone --mirror` wrote +// into refs/heads. +// +// Changing the refspec does not move refs a previous clone already made: they go +// on occupying the names refs/heads reserves for the sessions — `git checkout -b +// feature` fails with "a branch named 'feature' already exists" — and they stay +// frozen at whatever commit that clone saw. The fetch has just written the +// current value of each under refs/remotes/origin, so the copy is redundant. +// +// Two guards, because a session's own branch lives in refs/heads too: only names +// origin actually has are dropped, and never one a worktree has checked out. +// `git update-ref -d` deletes a checked-out branch without a word of complaint, +// and that branch is where a fix session's work lives before it is pushed. +func (w Workspace) dropFetchedHeads(ctx context.Context, path string) error { + heads, err := gitDir(ctx, path, "for-each-ref", "--format=%(refname:lstrip=2)", "refs/heads") + if err != nil { + return err + } + if heads == "" { + return nil // the usual case: a mirror crq made itself never writes refs/heads + } + live, err := checkedOutBranches(ctx, path) + if err != nil { + return err + } + for _, name := range strings.Split(heads, "\n") { + if live[name] { + continue + } + if _, err := gitDir(ctx, path, "show-ref", "--verify", "--quiet", "refs/remotes/origin/"+name); err != nil { + continue // origin has no such branch, so this one belongs to a session + } + if _, err := gitDir(ctx, path, "update-ref", "-d", "refs/heads/"+name); err != nil { + return err + } + } + return nil +} + +// checkedOutBranches is the set of branch names some worktree of the mirror at +// path currently has checked out. +func checkedOutBranches(ctx context.Context, path string) (map[string]bool, error) { + out, err := gitDir(ctx, path, "worktree", "list", "--porcelain") + if err != nil { + return nil, err + } + live := map[string]bool{} + for _, line := range strings.Split(out, "\n") { + if name, ok := strings.CutPrefix(strings.TrimSpace(line), "branch refs/heads/"); ok { + live[name] = true + } + } + return live, nil +} + // setConfig writes one config key of the mirror at path, unless it already holds -// value. +// exactly value and nothing else. // // git serializes configuration writes through config.lock, so two dispatches of // the same repository arriving together make one of them fail with "could not @@ -255,10 +328,16 @@ const originRefspec = "+refs/heads/*:refs/remotes/origin/*" func (w Workspace) setConfig(ctx context.Context, path, key, value string) error { var err error for attempt := 0; attempt < 3; attempt++ { - if cur, gerr := w.git(ctx, path, "config", "--get", key); gerr == nil && cur == value { + // --get-all, not --get: a key holding several values answers --get with the + // last of them, so the one shape that most needs rewriting — a mirror with + // a second remote.origin.fetch — would read as already current. + if cur, gerr := w.git(ctx, path, "config", "--get-all", key); gerr == nil && cur == value { return nil } - if _, err = w.git(ctx, path, "config", key, value); err == nil { + // --replace-all for the same reason: a plain single-value write refuses a + // multi-valued key outright ("cannot overwrite multiple values with a + // single value"), which would fail every later call for that repository. + if _, err = w.git(ctx, path, "config", "--replace-all", key, value); err == nil { return nil } if !isConfigLockContention(err) { @@ -270,7 +349,35 @@ func (w Workspace) setConfig(ctx context.Context, path, key, value string) error } // Still contended: the other worker is writing this same migration, so its // result is the one we wanted. Only its absence is worth an error. - if cur, gerr := w.git(ctx, path, "config", "--get", key); gerr == nil && cur == value { + if cur, gerr := w.git(ctx, path, "config", "--get-all", key); gerr == nil && cur == value { + return nil + } + return err +} + +// unsetConfig removes every value of one config key of the mirror at path. +// +// Read before writing and retry on contention, for setConfig's reasons. What +// differs is what a failure costs: a remote.origin.mirror that survives this +// turns a session's plain `git push` back into a mirror push, so the key's +// absence is verified rather than inferred from a command having run. +func (w Workspace) unsetConfig(ctx context.Context, path, key string) error { + var err error + for attempt := 0; attempt < 3; attempt++ { + if _, gerr := w.git(ctx, path, "config", "--get-all", key); gerr != nil { + return nil // not set, which is the normal case and worth no write at all + } + if _, err = w.git(ctx, path, "config", "--unset-all", key); err == nil { + return nil + } + if !isConfigLockContention(err) { + return err + } + if serr := sleepCtx(ctx, time.Duration(attempt+1)*200*time.Millisecond); serr != nil { + return serr + } + } + if _, gerr := w.git(ctx, path, "config", "--get-all", key); gerr != nil { return nil } return err diff --git a/internal/crq/workspace_test.go b/internal/crq/workspace_test.go index e9826b3..81bfcc3 100644 --- a/internal/crq/workspace_test.go +++ b/internal/crq/workspace_test.go @@ -563,6 +563,166 @@ func TestMirrorMigratesAnOldRefspec(t *testing.T) { } } +// A mirror an older crq made with `git clone --mirror` had already copied the +// remote's branches into refs/heads, and changing the fetch refspec does not +// move them: the name a session needs stays taken by a ref frozen at the commit +// that clone saw. +func TestMirrorDropsHeadsAnOldCloneLeftInPlace(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + origin := filepath.Join(base, repo) + sha := originRepo(t, origin) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + if _, err := gitDir(ctx, origin, "branch", "feature"); err != nil { + t.Fatal(err) + } + + // Lay the mirror out the way the older crq left it. + mirror, err := ws.mirrorPath(repo) + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(mirror), 0o700); err != nil { + t.Fatal(err) + } + if _, err := gitDir(ctx, "", "clone", "--mirror", "--quiet", ws.remoteURL(repo), mirror); err != nil { + t.Fatal(err) + } + // A branch of a session's own, which origin has never heard of. + if _, err := gitDir(ctx, mirror, "update-ref", "refs/heads/crq/fix-3", sha); err != nil { + t.Fatal(err) + } + + if _, err := ws.Mirror(ctx, repo); err != nil { + t.Fatal(err) + } + heads, err := gitDir(ctx, mirror, "for-each-ref", "--format=%(refname)", "refs/heads") + if err != nil { + t.Fatal(err) + } + if strings.Contains(heads, "refs/heads/feature") || strings.Contains(heads, "refs/heads/main") { + t.Errorf("refs/heads = %q, want the clone's copies of origin's branches gone", heads) + } + if !strings.Contains(heads, "refs/heads/crq/fix-3") { + t.Errorf("refs/heads = %q, want a session's own branch left alone", heads) + } + if remotes, err := gitDir(ctx, mirror, "for-each-ref", "--format=%(refname)", "refs/remotes/origin"); err != nil || !strings.Contains(remotes, "refs/remotes/origin/feature") { + t.Errorf("remote refs = %q err=%v, want origin's branches under origin", remotes, err) + } + + // The name is free for a session again — and stays the session's, even though + // origin has a branch of that name: `git update-ref -d` would delete a + // checked-out branch without complaint, work and all. + co, err := ws.Checkout(ctx, repo, 3, sha) + if err != nil { + t.Fatal(err) + } + if _, err := co.Git(ctx, "checkout", "-b", "feature"); err != nil { + t.Fatalf("a session could not create its branch: %v", err) + } + if _, err := ws.Mirror(ctx, repo); err != nil { + t.Fatal(err) + } + if branch, err := co.Git(ctx, "rev-parse", "--abbrev-ref", "HEAD"); err != nil || branch != "feature" { + t.Errorf("branch = %q err=%v, want the session's checked-out branch to survive", branch, err) + } +} + +// A mirror with a second remote.origin.fetch refused a single-value write — +// "cannot overwrite multiple values with a single value" — which would have +// failed every later Mirror call for that repository for good. +func TestMirrorMigratesAMultiValuedRefspec(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + originRepo(t, filepath.Join(base, repo)) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + + mirror, err := ws.Mirror(ctx, repo) + if err != nil { + t.Fatal(err) + } + // The refspec that reaches into refs/heads, added rather than replacing. + if _, err := gitDir(ctx, mirror, "config", "--add", "remote.origin.fetch", "+refs/*:refs/*"); err != nil { + t.Fatal(err) + } + if _, err := ws.Mirror(ctx, repo); err != nil { + t.Fatalf("a mirror with two refspecs could not be migrated: %v", err) + } + if got, err := gitDir(ctx, mirror, "config", "--get-all", "remote.origin.fetch"); err != nil || got != originRefspec { + t.Errorf("refspec = %q err=%v, want only %q", got, err, originRefspec) + } +} + +// 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. Retrying past it and handing back the +// mirror anyway presented refs known to be stale as current ones. +func TestMirrorReportsAFetchItCouldNotComplete(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + origin := filepath.Join(base, repo) + originRepo(t, origin) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + + mirror, err := ws.Mirror(ctx, repo) + if err != nil { + t.Fatal(err) + } + // Origin moves on, so the next fetch has a ref it must update... + if err := os.WriteFile(filepath.Join(origin, "README.md"), []byte("two\n"), 0o644); err != nil { + t.Fatal(err) + } + for _, args := range [][]string{{"add", "README.md"}, {"commit", "-m", "second"}} { + if _, err := gitDir(ctx, origin, args...); err != nil { + t.Fatal(err) + } + } + // ...and the lock makes that impossible for as long as it is there. + lock := filepath.Join(mirror, "refs", "remotes", "origin", "main.lock") + if err := os.MkdirAll(filepath.Dir(lock), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(lock, nil, 0o600); err != nil { + t.Fatal(err) + } + if _, err := ws.Mirror(ctx, repo); err == nil { + t.Error("a mirror that could not be fetched was reported as current") + } +} + +// Discarding the failure to clear remote.origin.mirror handed back a mirror +// whose later plain push still had mirror semantics — publishing internal refs +// and deleting remote branches, the exact hazard the migration exists for. +func TestMirrorReportsAPushMirrorFlagItCouldNotClear(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + originRepo(t, filepath.Join(base, repo)) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + + mirror, err := ws.Mirror(ctx, repo) + if err != nil { + t.Fatal(err) + } + if _, err := gitDir(ctx, mirror, "config", "remote.origin.mirror", "true"); err != nil { + t.Fatal(err) + } + // Somebody else holds the lock for good, the way a killed git does. + if err := os.WriteFile(filepath.Join(mirror, "config.lock"), nil, 0o600); err != nil { + t.Fatal(err) + } + defer os.Remove(filepath.Join(mirror, "config.lock")) + if _, err := ws.Mirror(ctx, repo); err == nil { + t.Error("a mirror still configured to push as a mirror was handed back as usable") + } +} + // git serializes config writes through config.lock, so an unconditional write on // every Mirror call made two dispatches of one repository collide with "could // not lock config file" — before the fetch, which is the part written to survive From 892525c6a464117aaf260dce4cd7f238f683c5ce Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 05:23:11 +0200 Subject: [PATCH 09/17] Keep a session's branch and collect a checkout stamped ahead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/crq/workspace.go | 42 +++++++++++++++++--- internal/crq/workspace_test.go | 72 ++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 6 deletions(-) diff --git a/internal/crq/workspace.go b/internal/crq/workspace.go index c044e4c..a9bdaac 100644 --- a/internal/crq/workspace.go +++ b/internal/crq/workspace.go @@ -271,10 +271,13 @@ func (w Workspace) fetchMirror(ctx context.Context, path string) error { // frozen at whatever commit that clone saw. The fetch has just written the // current value of each under refs/remotes/origin, so the copy is redundant. // -// Two guards, because a session's own branch lives in refs/heads too: only names -// origin actually has are dropped, and never one a worktree has checked out. -// `git update-ref -d` deletes a checked-out branch without a word of complaint, -// and that branch is where a fix session's work lives before it is pushed. +// Three guards, because a session's own branch lives in refs/heads too, and +// deleting that one loses the commits it is the only ref for: never a name a +// worktree has checked out (`git update-ref -d` deletes a checked-out branch +// without a word of complaint), only names origin actually has, and only when +// origin already has the commits. The last is what makes a deletion lossless +// rather than merely likely to be: checkout status alone would drop a session's +// branch the moment it detached HEAD to look at another commit. func (w Workspace) dropFetchedHeads(ctx context.Context, path string) error { heads, err := gitDir(ctx, path, "for-each-ref", "--format=%(refname:lstrip=2)", "refs/heads") if err != nil { @@ -294,6 +297,15 @@ func (w Workspace) dropFetchedHeads(ctx context.Context, path string) error { if _, err := gitDir(ctx, path, "show-ref", "--verify", "--quiet", "refs/remotes/origin/"+name); err != nil { continue // origin has no such branch, so this one belongs to a session } + // A clone's copy is at or behind what the fetch just wrote under + // refs/remotes/origin, so every commit it names survives its deletion. A + // branch holding commits origin has never seen is somebody's unpushed work + // — or a leftover of a force-push — and its ref is the only thing keeping + // those commits reachable. Keeping it costs one occupied name; dropping it + // costs the commits. + if _, err := gitDir(ctx, path, "merge-base", "--is-ancestor", "refs/heads/"+name, "refs/remotes/origin/"+name); err != nil { + continue + } if _, err := gitDir(ctx, path, "update-ref", "-d", "refs/heads/"+name); err != nil { return err } @@ -579,14 +591,32 @@ func pruneStaleWork(ctx context.Context, mirror, repoDir string) error { return nil } -// newestModTime is the most recent modification anywhere under dir. +// newestModTime is the most recent modification anywhere under dir that is not +// in the future. +// +// A file can carry a timestamp later than now — an extracted archive that kept +// the packer's clock, a build stamping ahead, a host whose clock was corrected +// backwards. `time.Since` of such a stamp is negative, which reads as "touched +// moments ago" for as long as the future lasts, so one bad timestamp would make +// an abandoned checkout immortal and every dispatch would rescan it. +// +// Ignored rather than clamped to now: clamping produces exactly the same +// "touched moments ago" answer on every later scan. A stamp in the future says +// nothing about when anybody last worked here, so the newest real modification +// is the honest answer — and a live session's own beat (keepAlive) is always one +// of those. func newestModTime(dir string) time.Time { + now := time.Now() newest := time.Time{} _ = filepath.WalkDir(dir, func(_ string, d os.DirEntry, err error) error { if err != nil { return nil } - if info, ierr := d.Info(); ierr == nil && info.ModTime().After(newest) { + info, ierr := d.Info() + if ierr != nil || info.ModTime().After(now) { + return nil + } + if info.ModTime().After(newest) { newest = info.ModTime() } return nil diff --git a/internal/crq/workspace_test.go b/internal/crq/workspace_test.go index 81bfcc3..4e918e6 100644 --- a/internal/crq/workspace_test.go +++ b/internal/crq/workspace_test.go @@ -336,6 +336,28 @@ func TestPruningMeasuresTheNewestFileNotTheDirectory(t *testing.T) { } } +// One file stamped in the future — an extracted artifact, a corrected clock — +// made time.Since negative and so always under staleWorkAge, and the abandoned +// checkout holding it would never be collected. +func TestPruningIgnoresATimestampInTheFuture(t *testing.T) { + dir := t.TempDir() + old := time.Now().Add(-2 * staleWorkAge) + future := filepath.Join(dir, "artifact.tar") + if err := os.WriteFile(future, []byte("packed elsewhere"), 0o644); err != nil { + t.Fatal(err) + } + ahead := time.Now().Add(90 * 24 * time.Hour) + if err := os.Chtimes(future, ahead, ahead); err != nil { + t.Fatal(err) + } + if err := os.Chtimes(dir, old, old); err != nil { + t.Fatal(err) + } + if since := time.Since(newestModTime(dir)); since < staleWorkAge { + t.Errorf("the checkout reads as %s old, want the future stamp ignored and the checkout collectable", since) + } +} + // `clone --bare` copies the remote's branch heads straight into refs/heads, and // a refspec set afterwards only governs later fetches. A session could then not // create its branch — the name is taken — or would land on the commit the clone @@ -630,6 +652,56 @@ func TestMirrorDropsHeadsAnOldCloneLeftInPlace(t *testing.T) { } } +// Checked-out status alone is not what tells a clone's leftover from a session's +// branch: a session that detaches HEAD to look at another commit still owns the +// branch it committed to, and deleting that ref is the only thing keeping its +// commits reachable. +func TestMirrorKeepsASessionBranchThatIsNotCheckedOut(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + origin := filepath.Join(base, repo) + sha := originRepo(t, origin) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + // Origin has the name too, which is what made the mirror read the session's + // branch as a copy of it. + if _, err := gitDir(ctx, origin, "branch", "feature"); err != nil { + t.Fatal(err) + } + + co, err := ws.Checkout(ctx, repo, 7, sha) + if err != nil { + t.Fatal(err) + } + for _, args := range [][]string{ + {"config", "user.email", "test@example.invalid"}, + {"config", "user.name", "crq test"}, + {"checkout", "-b", "feature"}, + {"commit", "--allow-empty", "-m", "unpushed work"}, + {"checkout", "--detach"}, + } { + if _, err := co.Git(ctx, args...); err != nil { + t.Fatalf("git %s: %v", strings.Join(args, " "), err) + } + } + work, err := co.Git(ctx, "rev-parse", "refs/heads/feature") + if err != nil { + t.Fatal(err) + } + + if _, err := ws.Mirror(ctx, repo); err != nil { + t.Fatal(err) + } + mirror, err := ws.mirrorPath(repo) + if err != nil { + t.Fatal(err) + } + if got, err := gitDir(ctx, mirror, "rev-parse", "refs/heads/feature"); err != nil || got != work { + t.Errorf("refs/heads/feature = %q err=%v, want the session's unpushed commit %s", got, err, work) + } +} + // A mirror with a second remote.origin.fetch refused a single-value write — // "cannot overwrite multiple values with a single value" — which would have // failed every later Mirror call for that repository for good. From 141e2845b49f5d386e9e4970f1ae4b84cec23ed6 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 06:09:41 +0200 Subject: [PATCH 10/17] Harden and isolate repository workspaces --- AGENTS.md | 6 +- internal/crq/workspace_wiring.go | 23 +++ internal/crq/workspace_wiring_test.go | 21 +++ internal/{crq => workspace}/workspace.go | 126 ++++++++++----- internal/{crq => workspace}/workspace_test.go | 148 ++++++++++++++++-- 5 files changed, 267 insertions(+), 57 deletions(-) create mode 100644 internal/crq/workspace_wiring.go create mode 100644 internal/crq/workspace_wiring_test.go rename internal/{crq => workspace}/workspace.go (89%) rename internal/{crq => workspace}/workspace_test.go (88%) diff --git a/AGENTS.md b/AGENTS.md index 399e946..3838db2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -8,7 +8,7 @@ CLI contract read `README.md` and `llms.txt`; for usage read `crq help`. ## Package layout Dependency rule (Go-enforced, no cycles): `dialect ← engine ← crq`, `state ← crq`, -`gh ← {state, crq}`. The engine does no I/O by construction. +`gh ← {state, crq}`, `workspace ← crq`. The engine does no I/O by construction. - `internal/dialect/` — ALL bot-text knowledge, zero deps. CodeRabbit/Codex completion, rate-limit, paused, in-progress, failed, summary-only-plan, @@ -26,6 +26,10 @@ Dependency rule (Go-enforced, no cycles): `dialect ← engine ← crq`, `state The only package (besides dialect) allowed to say "rate limit". `ListCheckRuns` fetches a ref's check runs (envelope-paged, ETag'd); matching them to a bot is dialect's `ClassifyCheckRun`, never gh's. +- `internal/workspace/` — reusable repository mirrors, detached PR worktrees, + credential-safe Git execution, stale-worktree pruning, and mirror migration. + Owns persistent filesystem and process I/O for checkouts; `crq` supplies only + configured roots and a current-token resolver. - `internal/state/` — persisted schema v3: one `Round` per PR, one global `FireSlot`, the CodeRabbit `AccountQuota`, an `Archive` ring. Round transition methods, the CAS store, and dashboard rendering. `Round.CoBots` holds per- diff --git a/internal/crq/workspace_wiring.go b/internal/crq/workspace_wiring.go new file mode 100644 index 0000000..595be61 --- /dev/null +++ b/internal/crq/workspace_wiring.go @@ -0,0 +1,23 @@ +package crq + +import ( + "context" + + ghapi "github.com/kristofferR/coderabbit-queue/internal/gh" + "github.com/kristofferR/coderabbit-queue/internal/workspace" +) + +// workspace wires crq's configured cache root and GitHub credential resolution +// into the reusable workspace implementation. +func (s *Service) workspace(context.Context) workspace.Workspace { + return workspace.Workspace{ + Root: s.cfg.WorkspaceRoot, + TokenSource: ghapi.LookupToken, + } +} + +// gitDir keeps command-local Git probes in crq while the process and error +// handling live with the rest of the Git implementation. +func gitDir(ctx context.Context, dir string, args ...string) (string, error) { + return workspace.Git(ctx, dir, args...) +} diff --git a/internal/crq/workspace_wiring_test.go b/internal/crq/workspace_wiring_test.go new file mode 100644 index 0000000..3e13362 --- /dev/null +++ b/internal/crq/workspace_wiring_test.go @@ -0,0 +1,21 @@ +package crq + +import ( + "context" + "testing" +) + +// The daemon's workspace has to come from the config file, not the process +// environment, and credentials must be resolved when Git runs so token rotation +// does not leave a long-lived checkout with a stale value. +func TestServiceWorkspaceUsesTheConfiguredRootAndCurrentToken(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "ghp_from_the_environment") + s := &Service{cfg: Config{WorkspaceRoot: "/tmp/crq-configured-root"}} + ws := s.workspace(context.Background()) + if ws.Root != "/tmp/crq-configured-root" { + t.Errorf("workspace root = %q, want the configured one", ws.Root) + } + if got := ws.TokenSource(context.Background()); got != "ghp_from_the_environment" { + t.Errorf("workspace token = %q, want the one the API client resolves", got) + } +} diff --git a/internal/crq/workspace.go b/internal/workspace/workspace.go similarity index 89% rename from internal/crq/workspace.go rename to internal/workspace/workspace.go index a9bdaac..2cb1844 100644 --- a/internal/crq/workspace.go +++ b/internal/workspace/workspace.go @@ -1,17 +1,18 @@ -package crq +package workspace import ( "context" + "crypto/rand" + "encoding/hex" "errors" "fmt" + "io" "os" "os/exec" "path/filepath" "strconv" "strings" "time" - - ghapi "github.com/kristofferR/coderabbit-queue/internal/gh" ) // Workspace is crq's own place on disk for a repository. @@ -40,6 +41,10 @@ type Workspace struct { // developer machine; a daemon configured with GITHUB_TOKEN alone has no // helper, and git does not read that variable by itself. Token string + // TokenSource resolves credentials immediately before each Git command. + // Long-lived checkouts use this instead of retaining the token that happened + // to be current when they were created. + TokenSource func(context.Context) string } // DefaultWorkspaceRoot is $XDG_CACHE_HOME/crq (or ~/.cache/crq). @@ -61,11 +66,15 @@ func DefaultWorkspaceRoot() (string, error) { // snippet that reads it, so a process listing, a log line and this package's own // error strings all carry the snippet and never the secret. func (w Workspace) git(ctx context.Context, dir string, args ...string) (string, error) { - if strings.TrimSpace(w.Token) == "" { + token := strings.TrimSpace(w.Token) + if w.TokenSource != nil { + token = strings.TrimSpace(w.TokenSource(ctx)) + } + if token == "" { return gitDir(ctx, dir, args...) } full := append([]string{"-c", "credential.helper=", "-c", "credential.helper=" + credentialHelper}, args...) - return gitEnv(ctx, dir, []string{"CRQ_GIT_TOKEN=" + w.Token}, full...) + return gitEnv(ctx, dir, []string{"CRQ_GIT_TOKEN=" + token}, full...) } // credentialHelper answers for https://github.com and nothing else. @@ -114,7 +123,7 @@ func (w Workspace) mirrorPath(repo string) (string, error) { // splitRepo splits owner/name, rejecting anything that could traverse a path. func splitRepo(repo string) (owner, name string, ok bool) { - owner, name, ok = strings.Cut(NormalizeRepo(repo), "/") + owner, name, ok = strings.Cut(normalizeRepo(repo), "/") if !ok || owner == "" || name == "" { return "", "", false } @@ -189,7 +198,7 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { if _, err := w.git(ctx, pending, "config", "remote.origin.fetch", originRefspec); err != nil { return "", fmt.Errorf("configuring %s: %w", repo, err) } - if _, err := w.git(ctx, pending, "fetch", "--prune", "origin"); err != nil { + if _, err := w.git(ctx, pending, "fetch", "--prune", "origin", originRefspec, tagRefspec); err != nil { return "", fmt.Errorf("cloning %s: %w", repo, err) } if err := os.Rename(pending, path); err != nil { @@ -207,9 +216,9 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { func (w Workspace) remoteURL(repo string) string { base := strings.TrimSpace(os.Getenv("CRQ_REMOTE_BASE")) if base == "" { - return "https://github.com/" + NormalizeRepo(repo) + ".git" + return "https://github.com/" + normalizeRepo(repo) + ".git" } - return strings.TrimRight(base, "/") + "/" + NormalizeRepo(repo) + return strings.TrimRight(base, "/") + "/" + normalizeRepo(repo) } // originRefspec keeps fetched refs out of refs/heads, which belongs to the @@ -217,6 +226,10 @@ func (w Workspace) remoteURL(repo string) string { // out in a worktree makes any fetch that would update it fail outright. const originRefspec = "+refs/heads/*:refs/remotes/origin/*" +// tagRefspec is explicit so --prune removes deleted tags, and forced so a +// release tag moved on the remote is refreshed instead of rejected as stale. +const tagRefspec = "+refs/tags/*:refs/tags/*" + // migrateMirror brings a mirror an older crq left behind up to the rules this // one depends on, and is a no-op on a mirror that already follows them. func (w Workspace) migrateMirror(ctx context.Context, path string) error { @@ -241,7 +254,7 @@ func (w Workspace) fetchMirror(ctx context.Context, path string) error { // current. Retry briefly rather than failing a dispatch over that. var err error for attempt := 0; attempt < 3; attempt++ { - if _, err = w.git(ctx, path, "fetch", "--prune", "origin"); err == nil { + if _, err = w.git(ctx, path, "fetch", "--prune", "origin", originRefspec, tagRefspec); err == nil { return nil } // Only contention is somebody else's success. Expired credentials, an @@ -273,11 +286,12 @@ func (w Workspace) fetchMirror(ctx context.Context, path string) error { // // Three guards, because a session's own branch lives in refs/heads too, and // deleting that one loses the commits it is the only ref for: never a name a -// worktree has checked out (`git update-ref -d` deletes a checked-out branch -// without a word of complaint), only names origin actually has, and only when -// origin already has the commits. The last is what makes a deletion lossless -// rather than merely likely to be: checkout status alone would drop a session's -// branch the moment it detached HEAD to look at another commit. +// worktree has checked out, only names origin actually has, and only when origin +// already has the commits. The last is what makes a deletion lossless rather +// than merely likely to be: checkout status alone would drop a session's branch +// the moment it detached HEAD to look at another commit. Deletion itself uses +// git branch rather than update-ref, so Git rechecks worktree occupancy after +// our snapshot and refuses a branch a session attached in the meantime. func (w Workspace) dropFetchedHeads(ctx context.Context, path string) error { heads, err := gitDir(ctx, path, "for-each-ref", "--format=%(refname:lstrip=2)", "refs/heads") if err != nil { @@ -306,13 +320,26 @@ func (w Workspace) dropFetchedHeads(ctx context.Context, path string) error { if _, err := gitDir(ctx, path, "merge-base", "--is-ancestor", "refs/heads/"+name, "refs/remotes/origin/"+name); err != nil { continue } - if _, err := gitDir(ctx, path, "update-ref", "-d", "refs/heads/"+name); err != nil { + if err := deleteFetchedHead(ctx, path, name); err != nil { return err } } return nil } +func deleteFetchedHead(ctx context.Context, path, name string) error { + // Unlike update-ref, branch refuses to delete a branch that became checked + // out after dropFetchedHeads took its occupancy snapshot. + if _, err := gitDir(ctx, path, "branch", "-D", "--", name); err != nil { + live, lerr := checkedOutBranches(ctx, path) + if lerr == nil && live[name] { + return nil + } + return err + } + return nil +} + // checkedOutBranches is the set of branch names some worktree of the mirror at // path currently has checked out. func checkedOutBranches(ctx context.Context, path string) (map[string]bool, error) { @@ -485,7 +512,7 @@ func (w Workspace) Checkout(ctx context.Context, repo string, pr int, sha string // got before: a checkout that ages out of the workspace on its own. alive, stop := context.WithCancel(ctx) go keepAlive(alive, dir, heartbeatInterval) - return Checkout{Dir: dir, Repo: NormalizeRepo(repo), PR: pr, mirror: mirror, ws: w, token: token, stop: stop}, nil + return Checkout{Dir: dir, Repo: normalizeRepo(repo), PR: pr, mirror: mirror, ws: w, token: token, stop: stop}, nil } // fetchPullRef fetches refs/pull//head when sha is not in the mirror yet. @@ -576,11 +603,14 @@ func pruneStaleWork(ctx context.Context, mirror, repoDir string) error { } for _, entry := range entries { dir := filepath.Join(repoDir, prDir.Name(), entry.Name()) - // The NEWEST file in the checkout, not the directory's own timestamp: - // editing a file or running a build inside leaves the root's mtime - // untouched, so a busy session would read as abandoned. An idle one - // keeps that root fresh itself (keepAlive). - if touched := newestModTime(dir); time.Since(touched) < staleWorkAge { + // A fresh root is the checkout's heartbeat, so it proves the session + // is live without recursively statting a potentially huge generated + // tree. Only stale roots need the deeper scan for recent edits. + now := time.Now() + if rootHeartbeatFresh(dir, now) { + continue + } + if touched := newestModTimeAt(dir, now); now.Sub(touched) < staleWorkAge { continue } if err := removeWorktree(ctx, mirror, dir); err != nil { @@ -591,6 +621,11 @@ func pruneStaleWork(ctx context.Context, mirror, repoDir string) error { return nil } +func rootHeartbeatFresh(dir string, now time.Time) bool { + info, err := os.Stat(dir) + return err == nil && !info.ModTime().After(now) && now.Sub(info.ModTime()) < staleWorkAge +} + // newestModTime is the most recent modification anywhere under dir that is not // in the future. // @@ -606,7 +641,10 @@ func pruneStaleWork(ctx context.Context, mirror, repoDir string) error { // is the honest answer — and a live session's own beat (keepAlive) is always one // of those. func newestModTime(dir string) time.Time { - now := time.Now() + return newestModTimeAt(dir, time.Now()) +} + +func newestModTimeAt(dir string, now time.Time) time.Time { newest := time.Time{} _ = filepath.WalkDir(dir, func(_ string, d os.DirEntry, err error) error { if err != nil { @@ -666,6 +704,11 @@ func gitDir(ctx context.Context, dir string, args ...string) (string, error) { return gitEnv(ctx, dir, nil, args...) } +// Git runs git in dir ("" means the process's own directory). +func Git(ctx context.Context, dir string, args ...string) (string, error) { + return gitDir(ctx, dir, args...) +} + // gitEnv is gitDir with extra environment entries. func gitEnv(ctx context.Context, dir string, env []string, args ...string) (string, error) { cmd := exec.CommandContext(ctx, "git", args...) @@ -685,22 +728,6 @@ func gitEnv(ctx context.Context, dir string, env []string, args ...string) (stri return strings.TrimSpace(string(out)), nil } -// workspace is crq's workspace as this configuration describes it: the root from -// the config file (not just the process environment) and the token crq already -// resolved for the API, so a daemon with GITHUB_TOKEN alone can still clone. -func (s *Service) workspace(ctx context.Context) Workspace { - return Workspace{Root: s.cfg.WorkspaceRoot, Token: gitToken(ctx)} -} - -// gitToken is the token to authenticate git with, or "" to leave git to the -// host's own credential helper. -// -// It uses the SAME resolution the API client does, including `gh auth token`. -// Reading only the environment meant the documented `gh auth login` setup — API -// calls working fine — still produced unauthenticated clones, so every private -// checkout failed at dispatch's first real act. -func gitToken(ctx context.Context) string { return ghapi.LookupToken(ctx) } - // sleepCtx waits, or returns early when the context ends. func sleepCtx(ctx context.Context, d time.Duration) error { timer := time.NewTimer(d) @@ -712,3 +739,24 @@ func sleepCtx(ctx context.Context, d time.Duration) error { return nil } } + +func normalizeRepo(repo string) string { + repo = strings.TrimSpace(repo) + repo = strings.TrimSuffix(repo, ".git") + return strings.ToLower(repo) +} + +func randomToken() string { + var buf [16]byte + if _, err := io.ReadFull(rand.Reader, buf[:]); err != nil { + return strconv.FormatInt(time.Now().UnixNano(), 16) + } + return hex.EncodeToString(buf[:]) +} + +func shortSHA(sha string) string { + if len(sha) > 9 { + return sha[:9] + } + return sha +} diff --git a/internal/crq/workspace_test.go b/internal/workspace/workspace_test.go similarity index 88% rename from internal/crq/workspace_test.go rename to internal/workspace/workspace_test.go index 4e918e6..3f50881 100644 --- a/internal/crq/workspace_test.go +++ b/internal/workspace/workspace_test.go @@ -1,4 +1,4 @@ -package crq +package workspace import ( "context" @@ -99,22 +99,6 @@ func TestWorkspaceChecksOutAHeadWithoutACheckout(t *testing.T) { } } -// The daemon's workspace has to come from the config file, not the process -// environment, and its token from the same resolution the API client uses: -// reading only the environment meant a documented `gh auth login` setup made API -// calls work while every private clone came back unauthenticated. -func TestServiceWorkspaceUsesTheConfiguredRootAndToken(t *testing.T) { - t.Setenv("GITHUB_TOKEN", "ghp_from_the_environment") - s := &Service{cfg: Config{WorkspaceRoot: "/tmp/crq-configured-root"}} - ws := s.workspace(context.Background()) - if ws.Root != "/tmp/crq-configured-root" { - t.Errorf("workspace root = %q, want the configured one", ws.Root) - } - if ws.Token != "ghp_from_the_environment" { - t.Errorf("workspace token = %q, want the one the API client resolves", ws.Token) - } -} - // A repo name is turned into path segments, so anything that could climb out of // the workspace root has to be refused rather than joined. func TestWorkspaceRefusesRepoNamesThatEscape(t *testing.T) { @@ -874,3 +858,133 @@ func TestALiveCheckoutKeepsItselfFromBeingPruned(t *testing.T) { } t.Error("an idle checkout still read as abandoned while its process was alive") } + +func TestCheckoutGitRefreshesRotatedCredentials(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + sha := originRepo(t, filepath.Join(base, repo)) + t.Setenv("CRQ_REMOTE_BASE", base) + token := "ghp_first" + ws := Workspace{ + Root: t.TempDir(), + TokenSource: func(context.Context) string { + return token + }, + } + co, err := ws.Checkout(context.Background(), repo, 13, sha) + if err != nil { + t.Fatal(err) + } + + token = "ghp_rotated" + out, err := co.Git( + context.Background(), + "-c", `alias.current-token=!f() { printf '%s' "$CRQ_GIT_TOKEN"; }; f`, + "current-token", + ) + if err != nil { + t.Fatal(err) + } + if out != token { + t.Errorf("checkout used token %q after rotation, want %q", out, token) + } +} + +func TestMirrorPrunesDeletedTagsAndRefreshesMovedTags(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + origin := filepath.Join(base, repo) + first := originRepo(t, origin) + ctx := context.Background() + if _, err := gitDir(ctx, origin, "tag", "deleted"); err != nil { + t.Fatal(err) + } + if _, err := gitDir(ctx, origin, "tag", "release"); err != nil { + t.Fatal(err) + } + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + mirror, err := ws.Mirror(ctx, repo) + if err != nil { + t.Fatal(err) + } + + if _, err := gitDir(ctx, origin, "tag", "-d", "deleted"); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(origin, "README.md"), []byte("two\n"), 0o644); err != nil { + t.Fatal(err) + } + for _, args := range [][]string{{"add", "README.md"}, {"commit", "-m", "second"}} { + if _, err := gitDir(ctx, origin, args...); err != nil { + t.Fatal(err) + } + } + second, err := gitDir(ctx, origin, "rev-parse", "HEAD") + if err != nil { + t.Fatal(err) + } + if _, err := gitDir(ctx, origin, "tag", "-f", "release", second); err != nil { + t.Fatal(err) + } + if _, err := ws.Mirror(ctx, repo); err != nil { + t.Fatal(err) + } + + if _, err := gitDir(ctx, mirror, "show-ref", "--verify", "--quiet", "refs/tags/deleted"); err == nil { + t.Error("deleted remote tag survived in the reused mirror") + } + if got, err := gitDir(ctx, mirror, "rev-parse", "refs/tags/release"); err != nil || got != second { + t.Errorf("release tag = %q err=%v, want moved tag %q (was %q)", got, err, second, first) + } +} + +func TestFetchedHeadDeletionRechecksWorktreeOccupancy(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + origin := filepath.Join(base, repo) + sha := originRepo(t, origin) + if _, err := gitDir(context.Background(), origin, "branch", "feature"); err != nil { + t.Fatal(err) + } + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + mirror, err := ws.Mirror(ctx, repo) + if err != nil { + t.Fatal(err) + } + if _, err := gitDir(ctx, mirror, "update-ref", "refs/heads/feature", sha); err != nil { + t.Fatal(err) + } + dir := filepath.Join(t.TempDir(), "work") + if _, err := gitDir(ctx, mirror, "worktree", "add", "--", dir, "feature"); err != nil { + t.Fatal(err) + } + + if err := deleteFetchedHead(ctx, mirror, "feature"); err != nil { + t.Fatalf("worktree-aware deletion rejected a branch a session attached: %v", err) + } + if got, err := gitDir(ctx, mirror, "rev-parse", "refs/heads/feature"); err != nil || got != sha { + t.Errorf("checked-out branch = %q err=%v, want it preserved at %q", got, err, sha) + } +} + +func TestFreshHeartbeatAvoidsDeepPruningDecision(t *testing.T) { + dir := t.TempDir() + old := time.Now().Add(-2 * staleWorkAge) + file := filepath.Join(dir, "generated.bin") + if err := os.WriteFile(file, []byte("large tree stand-in"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Chtimes(file, old, old); err != nil { + t.Fatal(err) + } + now := time.Now() + if err := os.Chtimes(dir, now, now); err != nil { + t.Fatal(err) + } + if !rootHeartbeatFresh(dir, now.Add(time.Second)) { + t.Fatal("fresh checkout root did not take the heartbeat fast path") + } +} From 7f0caf771c15330739d8598d9ea09bb069566c44 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 06:58:50 +0200 Subject: [PATCH 11/17] Leave the mirror usable by the caller that works in it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/workspace/workspace.go | 44 +++++++++++++++++++++++-- internal/workspace/workspace_test.go | 49 ++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 2 deletions(-) diff --git a/internal/workspace/workspace.go b/internal/workspace/workspace.go index 2cb1844..56a63d8 100644 --- a/internal/workspace/workspace.go +++ b/internal/workspace/workspace.go @@ -74,9 +74,14 @@ func (w Workspace) git(ctx context.Context, dir string, args ...string) (string, return gitDir(ctx, dir, args...) } full := append([]string{"-c", "credential.helper=", "-c", "credential.helper=" + credentialHelper}, args...) - return gitEnv(ctx, dir, []string{"CRQ_GIT_TOKEN=" + token}, full...) + return gitEnv(ctx, dir, []string{TokenEnv + "=" + token}, full...) } +// TokenEnv is the variable credentialHelper reads the token from. A caller that +// runs git in a checkout — rather than through this package — has to set it, so +// the name is part of the contract rather than an internal detail. +const TokenEnv = "CRQ_GIT_TOKEN" + // credentialHelper answers for https://github.com and nothing else. // // A git command can be led to a URL that repository CONTENT chose — a submodule, @@ -198,6 +203,12 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { if _, err := w.git(ctx, pending, "config", "remote.origin.fetch", originRefspec); err != nil { return "", fmt.Errorf("configuring %s: %w", repo, err) } + // Before the fetch, so the mirror is complete the moment it is renamed into + // place: another worker may pick it up as soon as it exists, and it would + // find one with no helper for its own git commands to use. + if err := w.persistCredentialHelper(ctx, pending); err != nil { + return "", fmt.Errorf("configuring %s: %w", repo, err) + } if _, err := w.git(ctx, pending, "fetch", "--prune", "origin", originRefspec, tagRefspec); err != nil { return "", fmt.Errorf("cloning %s: %w", repo, err) } @@ -244,7 +255,36 @@ func (w Workspace) migrateMirror(ctx context.Context, path string) error { // the refspec does not clear: a plain `git push` from a session's worktree // would then mirror the whole local ref namespace, publishing internal refs // and deleting remote branches this repository has never heard of. - return w.unsetConfig(ctx, path, "remote.origin.mirror") + if err := w.unsetConfig(ctx, path, "remote.origin.mirror"); err != nil { + return err + } + return w.persistCredentialHelper(ctx, path) +} + +// persistCredentialHelper writes the helper SNIPPET into the mirror's config, +// for the git commands this package does not run itself. +// +// A worktree is made for somebody else to work in, and that somebody runs a +// plain `git push`. Everything here injects the 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. +// +// The secret still never lands on disk: what is persisted READS TokenEnv from +// the environment, so only a caller that sets it can use it, and a mirror +// somebody else finds on disk hands out nothing. +func (w Workspace) persistCredentialHelper(ctx context.Context, path string) error { + if strings.TrimSpace(w.Token) == "" && w.TokenSource == nil { + return nil // the host's own helper answers; leave its configuration alone + } + // gitDir, not w.git: w.git injects a credential.helper of its own, so this + // read would answer with that injected value and conclude the mirror is + // already configured when its config is in fact empty. + if cur, err := gitDir(ctx, path, "config", "--local", "--get-all", "credential.helper"); err == nil && cur == credentialHelper { + return nil + } + _, err := gitDir(ctx, path, "config", "--local", "--replace-all", "credential.helper", credentialHelper) + return err } // fetchMirror brings the mirror at path up to date. diff --git a/internal/workspace/workspace_test.go b/internal/workspace/workspace_test.go index 3f50881..7947241 100644 --- a/internal/workspace/workspace_test.go +++ b/internal/workspace/workspace_test.go @@ -988,3 +988,52 @@ func TestFreshHeartbeatAvoidsDeepPruningDecision(t *testing.T) { t.Fatal("fresh checkout root did not take the heartbeat fast path") } } + +// A worktree is made for somebody else to work in, and that somebody pushes with +// a plain `git push`. crq's own commands pass the credential helper with -c, +// which lasts one command, so the mirror has to carry it for the ones this +// package does not run. +func TestMirrorPersistsTheCredentialHelperForOtherCallers(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + originRepo(t, filepath.Join(base, repo)) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir(), Token: "ghp_secret_value"} + ctx := context.Background() + + mirror, err := ws.Mirror(ctx, repo) + if err != nil { + t.Fatal(err) + } + got, err := gitDir(ctx, mirror, "config", "--local", "--get", "credential.helper") + if err != nil { + t.Fatalf("no credential helper persisted, so a session's own push has none: %v", err) + } + if got != credentialHelper { + t.Errorf("persisted credential.helper = %q, want the workspace helper", got) + } + // The snippet, never the secret: a mirror somebody else finds on disk must + // hand out nothing without TokenEnv set in the environment. + if strings.Contains(got, "ghp_secret_value") { + t.Error("the token itself was written into the mirror's config") + } +} + +// Without a token there is nothing to inject, and rewriting the host's own +// credential configuration would be crq overriding a choice that is not its own. +func TestMirrorLeavesTheHostsCredentialConfigurationAlone(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + originRepo(t, filepath.Join(base, repo)) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + + mirror, err := ws.Mirror(ctx, repo) + if err != nil { + t.Fatal(err) + } + if got, _ := gitDir(ctx, mirror, "config", "--local", "--get", "credential.helper"); got != "" { + t.Errorf("credential.helper = %q, want none written when crq has no token", got) + } +} From 06de1251b5ee16adaf05d07d25a56858b0c2a866 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 07:06:23 +0200 Subject: [PATCH 12/17] Preserve detached session branches during mirror migration --- internal/workspace/workspace.go | 50 +++++++++++++++++++-- internal/workspace/workspace_test.go | 66 ++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 3 deletions(-) diff --git a/internal/workspace/workspace.go b/internal/workspace/workspace.go index 56a63d8..006d7cb 100644 --- a/internal/workspace/workspace.go +++ b/internal/workspace/workspace.go @@ -160,6 +160,7 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { } } if _, err := os.Stat(filepath.Join(path, "HEAD")); err == nil { + dropFetched := w.hasLegacyFetchedHeads(ctx, path) if cerr := w.migrateMirror(ctx, path); cerr != nil { return "", fmt.Errorf("configuring %s: %w", repo, cerr) } @@ -169,8 +170,13 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { // After the fetch, because it is the fetch that puts the current value of // every remote branch under refs/remotes/origin — which is what makes the // copies an old clone left in refs/heads redundant. - if derr := w.dropFetchedHeads(ctx, path); derr != nil { - return "", fmt.Errorf("migrating %s: %w", repo, derr) + if dropFetched { + if derr := w.dropFetchedHeads(ctx, path); derr != nil { + return "", fmt.Errorf("migrating %s: %w", repo, derr) + } + } + if err := w.setConfig(ctx, path, fetchedHeadsMigratedKey, "true"); err != nil { + return "", fmt.Errorf("recording migration of %s: %w", repo, err) } return path, nil } else if !errors.Is(err, os.ErrNotExist) { @@ -212,6 +218,13 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { if _, err := w.git(ctx, pending, "fetch", "--prune", "origin", originRefspec, tagRefspec); err != nil { return "", fmt.Errorf("cloning %s: %w", repo, err) } + // A new mirror never copied remote branches into refs/heads. Record that + // before exposing it: from this point on that namespace belongs entirely to + // sessions, so no later refresh may mistake one of their branches for a + // legacy clone's copy. + if _, err := w.git(ctx, pending, "config", fetchedHeadsMigratedKey, "true"); err != nil { + return "", fmt.Errorf("configuring %s: %w", repo, err) + } if err := os.Rename(pending, path); err != nil { if _, statErr := os.Stat(filepath.Join(path, "HEAD")); statErr == nil { return path, nil // somebody else won the race; their mirror is as good @@ -241,6 +254,36 @@ const originRefspec = "+refs/heads/*:refs/remotes/origin/*" // release tag moved on the remote is refreshed instead of rejected as stale. const tagRefspec = "+refs/tags/*:refs/tags/*" +// fetchedHeadsMigratedKey records that refs/heads has already been handed over +// from an old clone to sessions. Without this marker, every refresh would keep +// applying migration heuristics to branches sessions created afterwards. +const fetchedHeadsMigratedKey = "crq.fetched-heads-migrated" + +// hasLegacyFetchedHeads identifies the one migration allowed to delete local +// branches. A marker proves it already ran; otherwise only old fetch/push +// mirror configuration proves refs/heads contains copies made by the clone. +// An unmarked mirror already using the current configuration may predate the +// marker, but its local branches belong to sessions and must be preserved. +func (w Workspace) hasLegacyFetchedHeads(ctx context.Context, path string) bool { + if migrated, err := w.git(ctx, path, "config", "--bool", "--get", fetchedHeadsMigratedKey); err == nil && migrated == "true" { + return false + } + if mirror, err := w.git(ctx, path, "config", "--bool", "--get", "remote.origin.mirror"); err == nil && mirror == "true" { + return true + } + fetch, err := w.git(ctx, path, "config", "--get-all", "remote.origin.fetch") + if err != nil { + return false + } + for _, refspec := range strings.Split(fetch, "\n") { + _, dst, ok := strings.Cut(strings.TrimPrefix(refspec, "+"), ":") + if ok && (dst == "refs/*" || dst == "refs/heads/*" || strings.HasPrefix(dst, "refs/heads/")) { + return true + } + } + return false +} + // migrateMirror brings a mirror an older crq left behind up to the rules this // one depends on, and is a no-op on a mirror that already follows them. func (w Workspace) migrateMirror(ctx context.Context, path string) error { @@ -316,7 +359,8 @@ func (w Workspace) fetchMirror(ctx context.Context, path string) error { } // dropFetchedHeads removes the branch copies an older `git clone --mirror` wrote -// into refs/heads. +// into refs/heads. It runs once for an unmarked mirror; after it succeeds, +// Mirror records fetchedHeadsMigratedKey and this namespace belongs to sessions. // // Changing the refspec does not move refs a previous clone already made: they go // on occupying the names refs/heads reserves for the sessions — `git checkout -b diff --git a/internal/workspace/workspace_test.go b/internal/workspace/workspace_test.go index 7947241..096801c 100644 --- a/internal/workspace/workspace_test.go +++ b/internal/workspace/workspace_test.go @@ -686,6 +686,72 @@ func TestMirrorKeepsASessionBranchThatIsNotCheckedOut(t *testing.T) { } } +// Once an old mirror's fetched heads have been removed, refs/heads belongs to +// sessions. An equal-tip branch is still session state: deleting it on every +// refresh also deletes its tracking configuration and reflog. +func TestMirrorKeepsAnEqualTipSessionBranchAfterMigration(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + origin := filepath.Join(base, repo) + sha := originRepo(t, origin) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + if _, err := gitDir(ctx, origin, "branch", "feature"); err != nil { + t.Fatal(err) + } + + // Exercise the legacy-clone migration before the session takes ownership of + // the name. Later Mirror calls must not run that cleanup against its branch. + mirror, err := ws.mirrorPath(repo) + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(mirror), 0o700); err != nil { + t.Fatal(err) + } + if _, err := gitDir(ctx, "", "clone", "--mirror", "--quiet", ws.remoteURL(repo), mirror); err != nil { + t.Fatal(err) + } + if _, err := ws.Mirror(ctx, repo); err != nil { + t.Fatal(err) + } + + co, err := ws.Checkout(ctx, repo, 7, sha) + if err != nil { + t.Fatal(err) + } + for _, args := range [][]string{ + {"checkout", "-b", "feature"}, + {"config", "branch.feature.remote", "origin"}, + {"config", "branch.feature.merge", "refs/heads/feature"}, + {"checkout", "--detach"}, + } { + if _, err := co.Git(ctx, args...); err != nil { + t.Fatalf("git %s: %v", strings.Join(args, " "), err) + } + } + if _, err := co.Git(ctx, "reflog", "exists", "refs/heads/feature"); err != nil { + t.Fatalf("session branch had no reflog before refresh: %v", err) + } + + if _, err := ws.Mirror(ctx, repo); err != nil { + t.Fatal(err) + } + if got, err := gitDir(ctx, mirror, "rev-parse", "refs/heads/feature"); err != nil || got != sha { + t.Errorf("refs/heads/feature = %q err=%v, want equal-tip session branch %s", got, err, sha) + } + if got, err := gitDir(ctx, mirror, "config", "--get", "branch.feature.remote"); err != nil || got != "origin" { + t.Errorf("branch.feature.remote = %q err=%v, want tracking configuration preserved", got, err) + } + if got, err := gitDir(ctx, mirror, "config", "--get", "branch.feature.merge"); err != nil || got != "refs/heads/feature" { + t.Errorf("branch.feature.merge = %q err=%v, want tracking configuration preserved", got, err) + } + if _, err := gitDir(ctx, mirror, "reflog", "exists", "refs/heads/feature"); err != nil { + t.Errorf("session branch reflog was deleted: %v", err) + } +} + // A mirror with a second remote.origin.fetch refused a single-value write — // "cannot overwrite multiple values with a single value" — which would have // failed every later Mirror call for that repository for good. From 89e76f5856204e22267b1aa1e06d962f2088cbce Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 07:10:46 +0200 Subject: [PATCH 13/17] Retry interrupted legacy head cleanup --- internal/workspace/workspace.go | 23 +++++++++---- internal/workspace/workspace_test.go | 50 ++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 7 deletions(-) diff --git a/internal/workspace/workspace.go b/internal/workspace/workspace.go index 006d7cb..a60de6e 100644 --- a/internal/workspace/workspace.go +++ b/internal/workspace/workspace.go @@ -161,6 +161,14 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { } if _, err := os.Stat(filepath.Join(path, "HEAD")); err == nil { dropFetched := w.hasLegacyFetchedHeads(ctx, path) + if dropFetched { + // Persist the pending state before migrateMirror clears the legacy + // configuration that identified these heads. A failed fetch or + // deletion must be retried by the next refresh. + if err := w.setConfig(ctx, path, fetchedHeadsMigratedKey, "false"); err != nil { + return "", fmt.Errorf("recording pending migration of %s: %w", repo, err) + } + } if cerr := w.migrateMirror(ctx, path); cerr != nil { return "", fmt.Errorf("configuring %s: %w", repo, cerr) } @@ -254,19 +262,20 @@ const originRefspec = "+refs/heads/*:refs/remotes/origin/*" // release tag moved on the remote is refreshed instead of rejected as stale. const tagRefspec = "+refs/tags/*:refs/tags/*" -// fetchedHeadsMigratedKey records that refs/heads has already been handed over -// from an old clone to sessions. Without this marker, every refresh would keep -// applying migration heuristics to branches sessions created afterwards. +// fetchedHeadsMigratedKey records whether refs/heads has been handed over from +// an old clone to sessions. False means cleanup is pending; true means later +// refreshes must not apply migration heuristics to session-created branches. const fetchedHeadsMigratedKey = "crq.fetched-heads-migrated" // hasLegacyFetchedHeads identifies the one migration allowed to delete local -// branches. A marker proves it already ran; otherwise only old fetch/push -// mirror configuration proves refs/heads contains copies made by the clone. +// branches. A false marker retries interrupted cleanup and a true marker proves +// it already ran; otherwise only old fetch/push mirror configuration proves +// refs/heads contains copies made by the clone. // An unmarked mirror already using the current configuration may predate the // marker, but its local branches belong to sessions and must be preserved. func (w Workspace) hasLegacyFetchedHeads(ctx context.Context, path string) bool { - if migrated, err := w.git(ctx, path, "config", "--bool", "--get", fetchedHeadsMigratedKey); err == nil && migrated == "true" { - return false + if migrated, err := w.git(ctx, path, "config", "--bool", "--get", fetchedHeadsMigratedKey); err == nil { + return migrated == "false" } if mirror, err := w.git(ctx, path, "config", "--bool", "--get", "remote.origin.mirror"); err == nil && mirror == "true" { return true diff --git a/internal/workspace/workspace_test.go b/internal/workspace/workspace_test.go index 096801c..6ecb7d1 100644 --- a/internal/workspace/workspace_test.go +++ b/internal/workspace/workspace_test.go @@ -779,6 +779,56 @@ func TestMirrorMigratesAMultiValuedRefspec(t *testing.T) { } } +// Normalizing a legacy mirror's configuration removes the evidence that its +// refs/heads came from clone --mirror. A pending marker must preserve that fact +// when deletion fails so the next refresh retries instead of abandoning cleanup. +func TestMirrorRetriesLegacyHeadCleanup(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + origin := filepath.Join(base, repo) + originRepo(t, origin) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + if _, err := gitDir(ctx, origin, "branch", "feature"); err != nil { + t.Fatal(err) + } + + mirror, err := ws.mirrorPath(repo) + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(mirror), 0o700); err != nil { + t.Fatal(err) + } + if _, err := gitDir(ctx, "", "clone", "--mirror", "--quiet", ws.remoteURL(repo), mirror); err != nil { + t.Fatal(err) + } + lock := filepath.Join(mirror, "refs", "heads", "feature.lock") + if err := os.WriteFile(lock, nil, 0o600); err != nil { + t.Fatal(err) + } + + if _, err := ws.Mirror(ctx, repo); err == nil { + t.Fatal("legacy head cleanup unexpectedly succeeded while its ref was locked") + } + if pending, err := gitDir(ctx, mirror, "config", "--bool", "--get", fetchedHeadsMigratedKey); err != nil || pending != "false" { + t.Fatalf("cleanup marker = %q err=%v, want pending", pending, err) + } + if err := os.Remove(lock); err != nil { + t.Fatal(err) + } + if _, err := ws.Mirror(ctx, repo); err != nil { + t.Fatalf("retrying legacy head cleanup: %v", err) + } + if heads, err := gitDir(ctx, mirror, "for-each-ref", "--format=%(refname)", "refs/heads"); err != nil || heads != "" { + t.Errorf("refs/heads = %q err=%v, want legacy heads removed by retry", heads, err) + } + if migrated, err := gitDir(ctx, mirror, "config", "--bool", "--get", fetchedHeadsMigratedKey); err != nil || migrated != "true" { + t.Errorf("cleanup marker = %q err=%v, want completed", migrated, err) + } +} + // 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. Retrying past it and handing back the // mirror anyway presented refs known to be stale as current ones. From 7958262b9a6dc52b74a918234faa8ad55d371af5 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 07:42:46 +0200 Subject: [PATCH 14/17] Protect active workspace checkouts --- internal/workspace/workspace.go | 31 ++++++++++++-- internal/workspace/workspace_test.go | 60 ++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 4 deletions(-) diff --git a/internal/workspace/workspace.go b/internal/workspace/workspace.go index a60de6e..9871a04 100644 --- a/internal/workspace/workspace.go +++ b/internal/workspace/workspace.go @@ -332,10 +332,26 @@ func (w Workspace) persistCredentialHelper(ctx context.Context, path string) err // gitDir, not w.git: w.git injects a credential.helper of its own, so this // read would answer with that injected value and conclude the mirror is // already configured when its config is in fact empty. - if cur, err := gitDir(ctx, path, "config", "--local", "--get-all", "credential.helper"); err == nil && cur == credentialHelper { + var err error + for attempt := 0; attempt < 3; attempt++ { + if cur, gerr := gitDir(ctx, path, "config", "--local", "--get-all", "credential.helper"); gerr == nil && cur == credentialHelper { + return nil + } + if _, err = gitDir(ctx, path, "config", "--local", "--replace-all", "credential.helper", credentialHelper); err == nil { + return nil + } + if !isConfigLockContention(err) { + return err + } + if serr := sleepCtx(ctx, time.Duration(attempt+1)*200*time.Millisecond); serr != nil { + return serr + } + } + // Another worker may have installed the same helper while this one waited + // for config.lock. Its value is the successful migration we wanted. + if cur, gerr := gitDir(ctx, path, "config", "--local", "--get-all", "credential.helper"); gerr == nil && cur == credentialHelper { return nil } - _, err := gitDir(ctx, path, "config", "--local", "--replace-all", "credential.helper", credentialHelper) return err } @@ -731,8 +747,9 @@ func rootHeartbeatFresh(dir string, now time.Time) bool { // Ignored rather than clamped to now: clamping produces exactly the same // "touched moments ago" answer on every later scan. A stamp in the future says // nothing about when anybody last worked here, so the newest real modification -// is the honest answer — and a live session's own beat (keepAlive) is always one -// of those. +// is the honest answer. If every stamp is in the future, though, the clock may +// have moved behind a live session's heartbeat; return now because its age is +// indeterminate and deleting it would risk losing active work. func newestModTime(dir string) time.Time { return newestModTimeAt(dir, time.Now()) } @@ -752,6 +769,12 @@ func newestModTimeAt(dir string, now time.Time) time.Time { } return nil }) + // A backward clock adjustment can put the entire live checkout in the + // future. With no usable timestamp its age is indeterminate, so preserve it + // rather than treating the zero time as ancient. + if newest.IsZero() { + return now + } return newest } diff --git a/internal/workspace/workspace_test.go b/internal/workspace/workspace_test.go index 6ecb7d1..a277a2b 100644 --- a/internal/workspace/workspace_test.go +++ b/internal/workspace/workspace_test.go @@ -342,6 +342,27 @@ func TestPruningIgnoresATimestampInTheFuture(t *testing.T) { } } +// When the host clock moves backwards, every timestamp in a recently created +// checkout can be in the future. With no trustworthy age, pruning must preserve +// the checkout rather than treating the zero time as ancient. +func TestPruningPreservesACheckoutWithOnlyFutureTimestamps(t *testing.T) { + dir := t.TempDir() + file := filepath.Join(dir, "edited.go") + if err := os.WriteFile(file, []byte("package main"), 0o644); err != nil { + t.Fatal(err) + } + now := time.Now() + future := now.Add(time.Hour) + for _, path := range []string{file, dir} { + if err := os.Chtimes(path, future, future); err != nil { + t.Fatal(err) + } + } + if got := newestModTimeAt(dir, now); got != now { + t.Errorf("newest modification = %s, want indeterminate checkout preserved at %s", got, now) + } +} + // `clone --bare` copies the remote's branch heads straight into refs/heads, and // a refspec set afterwards only governs later fetches. A session could then not // create its branch — the name is taken — or would land on the commit the clone @@ -1135,6 +1156,45 @@ func TestMirrorPersistsTheCredentialHelperForOtherCallers(t *testing.T) { } } +// Two dispatches can both observe an old mirror without the helper, then race +// to install it. Losing config.lock is harmless when the winner wrote the same +// value, so the loser must retry and verify the resulting configuration. +func TestCredentialHelperMigrationSurvivesConfigLockContention(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + originRepo(t, filepath.Join(base, repo)) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + + mirror, err := ws.Mirror(ctx, repo) + if err != nil { + t.Fatal(err) + } + lock := filepath.Join(mirror, "config.lock") + if err := os.WriteFile(lock, nil, 0o600); err != nil { + t.Fatal(err) + } + winner := make(chan error, 1) + go func() { + time.Sleep(50 * time.Millisecond) + if err := os.Remove(lock); err != nil { + winner <- err + return + } + _, err := gitDir(ctx, mirror, "config", "--local", "--replace-all", "credential.helper", credentialHelper) + winner <- err + }() + + withToken := Workspace{Token: "ghp_secret_value"} + if err := withToken.persistCredentialHelper(ctx, mirror); err != nil { + t.Errorf("concurrent credential-helper migration failed: %v", err) + } + if err := <-winner; err != nil { + t.Fatalf("concurrent config writer: %v", err) + } +} + // Without a token there is nothing to inject, and rewriting the host's own // credential configuration would be crq overriding a choice that is not its own. func TestMirrorLeavesTheHostsCredentialConfigurationAlone(t *testing.T) { From e0bf7f41e98a95adafeb0789696a6c987d7cf08a Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 09:45:11 +0200 Subject: [PATCH 15/17] Harden shared mirror refreshes --- internal/workspace/workspace.go | 24 +++++++- internal/workspace/workspace_test.go | 87 ++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 3 deletions(-) diff --git a/internal/workspace/workspace.go b/internal/workspace/workspace.go index 9871a04..6ed4c00 100644 --- a/internal/workspace/workspace.go +++ b/internal/workspace/workspace.go @@ -169,7 +169,7 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { return "", fmt.Errorf("recording pending migration of %s: %w", repo, err) } } - if cerr := w.migrateMirror(ctx, path); cerr != nil { + if cerr := w.migrateMirror(ctx, path, repo); cerr != nil { return "", fmt.Errorf("configuring %s: %w", repo, cerr) } if ferr := w.fetchMirror(ctx, path); ferr != nil { @@ -295,7 +295,13 @@ func (w Workspace) hasLegacyFetchedHeads(ctx context.Context, path string) bool // migrateMirror brings a mirror an older crq left behind up to the rules this // one depends on, and is a no-op on a mirror that already follows them. -func (w Workspace) migrateMirror(ctx context.Context, path string) error { +func (w Workspace) migrateMirror(ctx context.Context, path, repo string) error { + // Linked worktrees share the mirror's remote configuration. A session may + // repoint origin for its own push, so restore the repository this mirror + // belongs to before trusting origin for the next fetch. + if err := w.setConfig(ctx, path, "remote.origin.url", w.remoteURL(repo)); err != nil { + return err + } // Enforce the refspec on EVERY call, not only at clone time. A mirror // created before this rule still fetches +refs/*:refs/*, and one branch // created in a worktree then wedges every future fetch for the whole @@ -444,6 +450,11 @@ func deleteFetchedHead(ctx context.Context, path, name string) error { if lerr == nil && live[name] { return nil } + // A concurrent migration may have deleted the same legacy branch after + // this caller selected it. Its absence is the desired end state. + if _, rerr := gitDir(ctx, path, "show-ref", "--verify", "--quiet", "refs/heads/"+name); rerr != nil { + return nil + } return err } return nil @@ -613,7 +624,14 @@ func (w Workspace) Checkout(ctx context.Context, repo string, pr int, sha string // `--` before the positional arguments: a commit-ish beginning with a dash // would otherwise be read as an option, which is the shape behind a long line // of clone-and-checkout CVEs. - if _, err := w.git(ctx, mirror, "worktree", "add", "--detach", "--", dir, sha); err != nil { + // A linked checkout can change the mirror's shared configuration. Do not + // execute hooks or fsmonitor commands a previous session installed while + // materializing repository-controlled content for this one. + if _, err := w.git(ctx, mirror, + "-c", "core.hooksPath="+os.DevNull, + "-c", "core.fsmonitor=false", + "worktree", "add", "--detach", "--", dir, sha, + ); err != nil { return Checkout{}, fmt.Errorf("checking out %s@%s: %w", repo, shortSHA(sha), err) } // Held for as long as the caller's context lives, which is as long as this diff --git a/internal/workspace/workspace_test.go b/internal/workspace/workspace_test.go index a277a2b..854ba5d 100644 --- a/internal/workspace/workspace_test.go +++ b/internal/workspace/workspace_test.go @@ -425,6 +425,50 @@ func TestMirrorMigrationClearsThePushMirrorFlag(t *testing.T) { } } +// Linked worktrees share remote configuration with the mirror. A session may +// repoint origin for a fork push, but the next refresh still belongs to the +// base repository and must fetch from it. +func TestMirrorRefreshRestoresOriginURL(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + origin := filepath.Join(base, repo) + originRepo(t, origin) + other := filepath.Join(base, "fork/thing") + originRepo(t, other) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + + mirror, err := ws.Mirror(ctx, repo) + if err != nil { + t.Fatal(err) + } + if _, err := gitDir(ctx, mirror, "remote", "set-url", "origin", other); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(origin, "README.md"), []byte("base moved\n"), 0o644); err != nil { + t.Fatal(err) + } + for _, args := range [][]string{{"add", "README.md"}, {"commit", "-m", "base moved"}} { + if _, err := gitDir(ctx, origin, args...); err != nil { + t.Fatal(err) + } + } + want, err := gitDir(ctx, origin, "rev-parse", "HEAD") + if err != nil { + t.Fatal(err) + } + if _, err := ws.Mirror(ctx, repo); err != nil { + t.Fatal(err) + } + if got, err := gitDir(ctx, mirror, "remote", "get-url", "origin"); err != nil || got != ws.remoteURL(repo) { + t.Errorf("origin URL = %q err=%v, want %q", got, err, ws.remoteURL(repo)) + } + if got, err := gitDir(ctx, mirror, "rev-parse", "refs/remotes/origin/main"); err != nil || got != want { + t.Errorf("fetched main = %q err=%v, want base repository head %q", got, err, want) + } +} + // The mirror's HEAD exists because the mirror exists, so testing for it after a // failed fetch declared every failure contention: expired credentials and an // unreachable remote came back as success with stale refs, and the caller met @@ -1107,6 +1151,49 @@ func TestFetchedHeadDeletionRechecksWorktreeOccupancy(t *testing.T) { } } +func TestFetchedHeadDeletionToleratesAConcurrentDeletion(t *testing.T) { + ctx := context.Background() + repo := t.TempDir() + if _, err := gitDir(ctx, "", "init", "--bare", "--quiet", repo); err != nil { + t.Fatal(err) + } + + if err := deleteFetchedHead(ctx, repo, "already-removed"); err != nil { + t.Fatalf("deleting an already-removed legacy branch: %v", err) + } +} + +// A checkout can install executable Git configuration in the shared mirror. +// Later worktree creation must not run that session's hook in the daemon. +func TestCheckoutDisablesSharedExecutableConfiguration(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + sha := originRepo(t, filepath.Join(base, repo)) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + mirror, err := ws.Mirror(ctx, repo) + if err != nil { + t.Fatal(err) + } + hooks := t.TempDir() + marker := filepath.Join(t.TempDir(), "post-checkout-ran") + hook := filepath.Join(hooks, "post-checkout") + if err := os.WriteFile(hook, []byte("#!/bin/sh\ntouch \""+marker+"\"\n"), 0o700); err != nil { + t.Fatal(err) + } + if _, err := gitDir(ctx, mirror, "config", "core.hooksPath", hooks); err != nil { + t.Fatal(err) + } + + if _, err := ws.Checkout(ctx, repo, 14, sha); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(marker); !os.IsNotExist(err) { + t.Errorf("shared post-checkout hook ran: %v", err) + } +} + func TestFreshHeartbeatAvoidsDeepPruningDecision(t *testing.T) { dir := t.TempDir() old := time.Now().Add(-2 * staleWorkAge) From f5081669836313148ef9e33299c8f612a55ba3e5 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 10:20:41 +0200 Subject: [PATCH 16/17] Isolate shared workspace Git state --- internal/workspace/workspace.go | 36 +++++++++++++---------- internal/workspace/workspace_test.go | 44 ++++++++++++++++++++++++---- 2 files changed, 60 insertions(+), 20 deletions(-) diff --git a/internal/workspace/workspace.go b/internal/workspace/workspace.go index 6ed4c00..0384be8 100644 --- a/internal/workspace/workspace.go +++ b/internal/workspace/workspace.go @@ -73,7 +73,15 @@ func (w Workspace) git(ctx context.Context, dir string, args ...string) (string, if token == "" { return gitDir(ctx, dir, args...) } - full := append([]string{"-c", "credential.helper=", "-c", "credential.helper=" + credentialHelper}, args...) + // A linked checkout can write executable configuration into the shared + // mirror. Do not let hooks or an fsmonitor command inherit the token from a + // later workspace operation. + full := append([]string{ + "-c", "core.hooksPath=" + os.DevNull, + "-c", "core.fsmonitor=false", + "-c", "credential.helper=", + "-c", "credential.helper=" + credentialHelper, + }, args...) return gitEnv(ctx, dir, []string{TokenEnv + "=" + token}, full...) } @@ -169,10 +177,10 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { return "", fmt.Errorf("recording pending migration of %s: %w", repo, err) } } - if cerr := w.migrateMirror(ctx, path, repo); cerr != nil { + if cerr := w.migrateMirror(ctx, path); cerr != nil { return "", fmt.Errorf("configuring %s: %w", repo, cerr) } - if ferr := w.fetchMirror(ctx, path); ferr != nil { + if ferr := w.fetchMirror(ctx, path, repo); ferr != nil { return "", fmt.Errorf("fetching %s: %w", repo, ferr) } // After the fetch, because it is the fetch that puts the current value of @@ -295,13 +303,7 @@ func (w Workspace) hasLegacyFetchedHeads(ctx context.Context, path string) bool // migrateMirror brings a mirror an older crq left behind up to the rules this // one depends on, and is a no-op on a mirror that already follows them. -func (w Workspace) migrateMirror(ctx context.Context, path, repo string) error { - // Linked worktrees share the mirror's remote configuration. A session may - // repoint origin for its own push, so restore the repository this mirror - // belongs to before trusting origin for the next fetch. - if err := w.setConfig(ctx, path, "remote.origin.url", w.remoteURL(repo)); err != nil { - return err - } +func (w Workspace) migrateMirror(ctx context.Context, path string) error { // Enforce the refspec on EVERY call, not only at clone time. A mirror // created before this rule still fetches +refs/*:refs/*, and one branch // created in a worktree then wedges every future fetch for the whole @@ -362,13 +364,17 @@ func (w Workspace) persistCredentialHelper(ctx context.Context, path string) err } // fetchMirror brings the mirror at path up to date. -func (w Workspace) fetchMirror(ctx context.Context, path string) error { +func (w Workspace) fetchMirror(ctx context.Context, path, repo string) error { // Two workers fetching one mirror race on git's ref locks, and the loser // reports "cannot lock ref" even though the winner has just made the mirror // current. Retry briefly rather than failing a dispatch over that. var err error for attempt := 0; attempt < 3; attempt++ { - if _, err = w.git(ctx, path, "fetch", "--prune", "origin", originRefspec, tagRefspec); err == nil { + // Use the mirror's repository explicitly. origin is shared with linked + // checkouts, and a fork session may have repointed it for its own push. + // Refreshing the mirror must neither trust nor rewrite that session's + // push destination. + if _, err = w.git(ctx, path, "fetch", "--prune", w.remoteURL(repo), originRefspec, tagRefspec); err == nil { return nil } // Only contention is somebody else's success. Expired credentials, an @@ -615,7 +621,7 @@ func (w Workspace) Checkout(ctx context.Context, repo string, pr int, sha string if err := pruneStaleWork(ctx, mirror, filepath.Dir(prDir)); err != nil { return Checkout{}, err } - w.fetchPullRef(ctx, mirror, pr, sha) + w.fetchPullRef(ctx, mirror, repo, pr, sha) token := randomToken() dir := filepath.Join(prDir, token) if err := os.MkdirAll(prDir, 0o700); err != nil { @@ -649,7 +655,7 @@ func (w Workspace) Checkout(ctx context.Context, repo string, pr int, sha string // unknown commit. GitHub publishes it as refs/pull//head. Best effort on // purpose: the checkout that follows is the real check, and its error names the // commit that could not be found. -func (w Workspace) fetchPullRef(ctx context.Context, mirror string, pr int, sha string) { +func (w Workspace) fetchPullRef(ctx context.Context, mirror, repo string, pr int, sha string) { if pr <= 0 { return } @@ -657,7 +663,7 @@ func (w Workspace) fetchPullRef(ctx context.Context, mirror string, pr int, sha return } ref := fmt.Sprintf("+refs/pull/%d/head:refs/remotes/origin/pull/%d", pr, pr) - _, _ = w.git(ctx, mirror, "fetch", "origin", ref) + _, _ = w.git(ctx, mirror, "fetch", w.remoteURL(repo), ref) } // workPath is the directory holding this PR's checkouts. Owner and name stay diff --git a/internal/workspace/workspace_test.go b/internal/workspace/workspace_test.go index 854ba5d..2436cb8 100644 --- a/internal/workspace/workspace_test.go +++ b/internal/workspace/workspace_test.go @@ -426,9 +426,9 @@ func TestMirrorMigrationClearsThePushMirrorFlag(t *testing.T) { } // Linked worktrees share remote configuration with the mirror. A session may -// repoint origin for a fork push, but the next refresh still belongs to the -// base repository and must fetch from it. -func TestMirrorRefreshRestoresOriginURL(t *testing.T) { +// repoint origin for a fork push, but a base-repository refresh must neither +// trust nor rewrite that session's push destination. +func TestMirrorRefreshLeavesOriginURLAlone(t *testing.T) { base := t.TempDir() repo := "owner/thing" origin := filepath.Join(base, repo) @@ -461,8 +461,8 @@ func TestMirrorRefreshRestoresOriginURL(t *testing.T) { if _, err := ws.Mirror(ctx, repo); err != nil { t.Fatal(err) } - if got, err := gitDir(ctx, mirror, "remote", "get-url", "origin"); err != nil || got != ws.remoteURL(repo) { - t.Errorf("origin URL = %q err=%v, want %q", got, err, ws.remoteURL(repo)) + if got, err := gitDir(ctx, mirror, "remote", "get-url", "origin"); err != nil || got != other { + t.Errorf("origin URL = %q err=%v, want the session's %q left alone", got, err, other) } if got, err := gitDir(ctx, mirror, "rev-parse", "refs/remotes/origin/main"); err != nil || got != want { t.Errorf("fetched main = %q err=%v, want base repository head %q", got, err, want) @@ -596,6 +596,40 @@ func TestCheckoutGitCarriesTheWorkspaceCredentials(t *testing.T) { } } +// A checkout can install executable Git configuration in the shared mirror. +// Later credential-bearing commands must not expose another session's token to +// that configuration. +func TestCheckoutGitProtectsCredentialsFromSharedHooks(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + sha := originRepo(t, filepath.Join(base, repo)) + t.Setenv("CRQ_REMOTE_BASE", base) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + co, err := Workspace{Root: t.TempDir(), Token: "ghp_secret_value"}.Checkout(ctx, repo, 12, sha) + if err != nil { + t.Fatal(err) + } + hooks := t.TempDir() + marker := filepath.Join(t.TempDir(), "token") + hook := filepath.Join(hooks, "pre-push") + script := "#!/bin/sh\nprintf '%s' \"$CRQ_GIT_TOKEN\" >\"" + marker + "\"\n" + if err := os.WriteFile(hook, []byte(script), 0o700); err != nil { + t.Fatal(err) + } + if _, err := gitDir(ctx, co.Dir, "config", "core.hooksPath", hooks); err != nil { + t.Fatal(err) + } + + if _, err := co.Git(ctx, "push", "--dry-run", "origin", "HEAD:refs/heads/token-test"); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(marker); !os.IsNotExist(err) { + t.Errorf("shared pre-push hook saw a later checkout's token: %v", err) + } +} + // A mirror created before the refspec rule still fetches +refs/*:refs/*. One // branch created in a worktree then wedges every future fetch for the whole // repository — "refusing to fetch into branch ... checked out at" — which is how From fd760d3b251b88d7ba2df1b3ad2931ac7bf6762b Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 11:11:31 +0200 Subject: [PATCH 17/17] Harden shared workspace isolation --- internal/workspace/workspace.go | 243 ++++++++++++++++++++++----- internal/workspace/workspace_test.go | 235 ++++++++++++++++++++++++-- 2 files changed, 419 insertions(+), 59 deletions(-) diff --git a/internal/workspace/workspace.go b/internal/workspace/workspace.go index 0384be8..12317ed 100644 --- a/internal/workspace/workspace.go +++ b/internal/workspace/workspace.go @@ -12,6 +12,7 @@ import ( "path/filepath" "strconv" "strings" + "syscall" "time" ) @@ -73,12 +74,7 @@ func (w Workspace) git(ctx context.Context, dir string, args ...string) (string, if token == "" { return gitDir(ctx, dir, args...) } - // A linked checkout can write executable configuration into the shared - // mirror. Do not let hooks or an fsmonitor command inherit the token from a - // later workspace operation. full := append([]string{ - "-c", "core.hooksPath=" + os.DevNull, - "-c", "core.fsmonitor=false", "-c", "credential.helper=", "-c", "credential.helper=" + credentialHelper, }, args...) @@ -168,31 +164,8 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { } } if _, err := os.Stat(filepath.Join(path, "HEAD")); err == nil { - dropFetched := w.hasLegacyFetchedHeads(ctx, path) - if dropFetched { - // Persist the pending state before migrateMirror clears the legacy - // configuration that identified these heads. A failed fetch or - // deletion must be retried by the next refresh. - if err := w.setConfig(ctx, path, fetchedHeadsMigratedKey, "false"); err != nil { - return "", fmt.Errorf("recording pending migration of %s: %w", repo, err) - } - } - if cerr := w.migrateMirror(ctx, path); cerr != nil { - return "", fmt.Errorf("configuring %s: %w", repo, cerr) - } - if ferr := w.fetchMirror(ctx, path, repo); ferr != nil { - return "", fmt.Errorf("fetching %s: %w", repo, ferr) - } - // After the fetch, because it is the fetch that puts the current value of - // every remote branch under refs/remotes/origin — which is what makes the - // copies an old clone left in refs/heads redundant. - if dropFetched { - if derr := w.dropFetchedHeads(ctx, path); derr != nil { - return "", fmt.Errorf("migrating %s: %w", repo, derr) - } - } - if err := w.setConfig(ctx, path, fetchedHeadsMigratedKey, "true"); err != nil { - return "", fmt.Errorf("recording migration of %s: %w", repo, err) + if err := w.refreshMirror(ctx, path, repo); err != nil { + return "", err } return path, nil } else if !errors.Is(err, os.ErrNotExist) { @@ -225,13 +198,16 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { if _, err := w.git(ctx, pending, "config", "remote.origin.fetch", originRefspec); err != nil { return "", fmt.Errorf("configuring %s: %w", repo, err) } + if err := w.enableWorktreeConfig(ctx, pending); err != nil { + return "", fmt.Errorf("configuring %s: %w", repo, err) + } // Before the fetch, so the mirror is complete the moment it is renamed into // place: another worker may pick it up as soon as it exists, and it would // find one with no helper for its own git commands to use. if err := w.persistCredentialHelper(ctx, pending); err != nil { return "", fmt.Errorf("configuring %s: %w", repo, err) } - if _, err := w.git(ctx, pending, "fetch", "--prune", "origin", originRefspec, tagRefspec); err != nil { + if _, err := w.git(ctx, pending, "fetch", "--prune", "--no-tags", w.remoteURL(repo), originRefspec, tagRefspec); err != nil { return "", fmt.Errorf("cloning %s: %w", repo, err) } // A new mirror never copied remote branches into refs/heads. Record that @@ -250,6 +226,103 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { return path, nil } +func (w Workspace) refreshMirror(ctx context.Context, path, repo string) error { + // A true marker proves refs/heads already belongs to sessions. The fast path + // avoids serializing ordinary refreshes after the one-time migration. + if w.fetchedHeadsMigrationComplete(ctx, path) && w.worktreeConfigComplete(ctx, path) { + return w.refreshMigratedMirror(ctx, path, repo) + } + + unlock, err := lockMirrorMigration(ctx, path) + if err != nil { + return fmt.Errorf("locking migration of %s: %w", repo, err) + } + defer unlock() + + // Another process may have completed the migration while this one waited. + if w.fetchedHeadsMigrationComplete(ctx, path) && w.worktreeConfigComplete(ctx, path) { + return w.refreshMigratedMirror(ctx, path, repo) + } + + dropFetched := w.hasLegacyFetchedHeads(ctx, path) + if dropFetched { + // Persist the pending state before migrateMirror clears the legacy + // configuration that identified these heads. A failed fetch or deletion + // must be retried by the next refresh. + if err := w.setConfig(ctx, path, fetchedHeadsMigratedKey, "false"); err != nil { + return fmt.Errorf("recording pending migration of %s: %w", repo, err) + } + } + if err := w.migrateMirror(ctx, path); err != nil { + return fmt.Errorf("configuring %s: %w", repo, err) + } + if err := w.fetchMirror(ctx, path, repo); err != nil { + return fmt.Errorf("fetching %s: %w", repo, err) + } + // The lock keeps a second refresh from acting on a stale migration decision + // or returning a checkout while legacy refs are still eligible for deletion. + if dropFetched { + if err := w.dropFetchedHeads(ctx, path); err != nil { + return fmt.Errorf("migrating %s: %w", repo, err) + } + } + if err := w.setConfig(ctx, path, fetchedHeadsMigratedKey, "true"); err != nil { + return fmt.Errorf("recording migration of %s: %w", repo, err) + } + return nil +} + +func (w Workspace) refreshMigratedMirror(ctx context.Context, path, repo string) error { + if err := w.migrateMirror(ctx, path); err != nil { + return fmt.Errorf("configuring %s: %w", repo, err) + } + if err := w.fetchMirror(ctx, path, repo); err != nil { + return fmt.Errorf("fetching %s: %w", repo, err) + } + return nil +} + +func (w Workspace) fetchedHeadsMigrationComplete(ctx context.Context, path string) bool { + migrated, err := w.git(ctx, path, "config", "--bool", "--get", fetchedHeadsMigratedKey) + return err == nil && migrated == "true" +} + +func (w Workspace) worktreeConfigComplete(ctx context.Context, path string) bool { + enabled, err := w.git(ctx, path, "config", "--bool", "--get", "extensions.worktreeConfig") + if err != nil || enabled != "true" { + return false + } + if _, err := w.git(ctx, path, "config", "--local", "--get", "core.bare"); err == nil { + return false + } + bare, err := w.git(ctx, path, "config", "--worktree", "--bool", "--get", "core.bare") + return err == nil && bare == "true" +} + +func lockMirrorMigration(ctx context.Context, path string) (func(), error) { + file, err := os.OpenFile(filepath.Join(path, "crq-migration.lock"), os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + return nil, err + } + for { + err = syscall.Flock(int(file.Fd()), syscall.LOCK_EX|syscall.LOCK_NB) + if err == nil { + return func() { + _ = syscall.Flock(int(file.Fd()), syscall.LOCK_UN) + _ = file.Close() + }, nil + } + if !errors.Is(err, syscall.EWOULDBLOCK) && !errors.Is(err, syscall.EAGAIN) { + _ = file.Close() + return nil, err + } + if err := sleepCtx(ctx, 100*time.Millisecond); err != nil { + _ = file.Close() + return nil, err + } + } +} + // remoteURL is the URL crq clones from. CRQ_REMOTE_BASE exists so a test can // point at a local path; in production it is github.com over https, which is // what the host's credential helper is configured for. @@ -266,9 +339,10 @@ func (w Workspace) remoteURL(repo string) string { // out in a worktree makes any fetch that would update it fail outright. const originRefspec = "+refs/heads/*:refs/remotes/origin/*" -// tagRefspec is explicit so --prune removes deleted tags, and forced so a -// release tag moved on the remote is refreshed instead of rejected as stale. -const tagRefspec = "+refs/tags/*:refs/tags/*" +// Remote tags live outside refs/tags, which belongs to sessions just like +// refs/heads. This lets refreshes prune and force-update the remote's view +// without deleting or moving tags a live checkout created. +const tagRefspec = "+refs/tags/*:refs/crq/remotes/origin/tags/*" // fetchedHeadsMigratedKey records whether refs/heads has been handed over from // an old clone to sessions. False means cleanup is pending; true means later @@ -304,6 +378,9 @@ func (w Workspace) hasLegacyFetchedHeads(ctx context.Context, path string) bool // migrateMirror brings a mirror an older crq left behind up to the rules this // one depends on, and is a no-op on a mirror that already follows them. func (w Workspace) migrateMirror(ctx context.Context, path string) error { + if err := w.enableWorktreeConfig(ctx, path); err != nil { + return err + } // Enforce the refspec on EVERY call, not only at clone time. A mirror // created before this rule still fetches +refs/*:refs/*, and one branch // created in a worktree then wedges every future fetch for the whole @@ -321,6 +398,34 @@ func (w Workspace) migrateMirror(ctx context.Context, path string) error { return w.persistCredentialHelper(ctx, path) } +func (w Workspace) enableWorktreeConfig(ctx context.Context, path string) error { + // Push URLs are worktree-specific. Enabling that extension requires moving + // core.bare into the main worktree's config; otherwise linked worktrees also + // inherit bare=true and Git refuses commands that need a working tree. + if w.worktreeConfigComplete(ctx, path) { + return nil + } + if err := w.setConfig(ctx, path, "extensions.worktreeConfig", "true"); err != nil { + return err + } + if _, err := w.git(ctx, path, "config", "--worktree", "--replace-all", "core.bare", "true"); err != nil { + return err + } + bare, err := w.git(ctx, path, "config", "--worktree", "--bool", "--get", "core.bare") + if err != nil { + return fmt.Errorf("verifying worktree core.bare: %w", err) + } + if bare != "true" { + return fmt.Errorf("verifying worktree core.bare: got %q, want true", bare) + } + if _, err := w.git(ctx, path, "config", "--local", "--get", "core.bare"); err == nil { + if _, err := w.git(ctx, path, "config", "--local", "--unset-all", "core.bare"); err != nil { + return err + } + } + return nil +} + // persistCredentialHelper writes the helper SNIPPET into the mirror's config, // for the git commands this package does not run itself. // @@ -370,11 +475,10 @@ func (w Workspace) fetchMirror(ctx context.Context, path, repo string) error { // current. Retry briefly rather than failing a dispatch over that. var err error for attempt := 0; attempt < 3; attempt++ { - // Use the mirror's repository explicitly. origin is shared with linked - // checkouts, and a fork session may have repointed it for its own push. - // Refreshing the mirror must neither trust nor rewrite that session's - // push destination. - if _, err = w.git(ctx, path, "fetch", "--prune", w.remoteURL(repo), originRefspec, tagRefspec); err == nil { + // Use the mirror's repository explicitly. Older checkouts may have + // repointed the shared origin before push URLs became worktree-local. + // Refreshing must neither trust nor rewrite that legacy state. + if _, err = w.git(ctx, path, "fetch", "--prune", "--no-tags", w.remoteURL(repo), originRefspec, tagRefspec); err == nil { return nil } // Only contention is somebody else's success. Expired credentials, an @@ -640,6 +744,10 @@ func (w Workspace) Checkout(ctx context.Context, repo string, pr int, sha string ); err != nil { return Checkout{}, fmt.Errorf("checking out %s@%s: %w", repo, shortSHA(sha), err) } + if _, err := w.git(ctx, dir, "config", "--worktree", "--replace-all", "remote.origin.pushurl", w.remoteURL(repo)); err != nil { + _ = removeWorktree(ctx, mirror, dir) + return Checkout{}, fmt.Errorf("configuring push target for %s: %w", repo, err) + } // Held for as long as the caller's context lives, which is as long as this // handle is worth anything. A caller whose context ends at once gets what it // got before: a checkout that ages out of the workspace on its own. @@ -663,7 +771,7 @@ func (w Workspace) fetchPullRef(ctx context.Context, mirror, repo string, pr int return } ref := fmt.Sprintf("+refs/pull/%d/head:refs/remotes/origin/pull/%d", pr, pr) - _, _ = w.git(ctx, mirror, "fetch", w.remoteURL(repo), ref) + _, _ = w.git(ctx, mirror, "fetch", "--no-tags", w.remoteURL(repo), ref) } // workPath is the directory holding this PR's checkouts. Owner and name stay @@ -756,7 +864,7 @@ func pruneStaleWork(ctx context.Context, mirror, repoDir string) error { func rootHeartbeatFresh(dir string, now time.Time) bool { info, err := os.Stat(dir) - return err == nil && !info.ModTime().After(now) && now.Sub(info.ModTime()) < staleWorkAge + return err == nil && (info.ModTime().After(now) || now.Sub(info.ModTime()) < staleWorkAge) } // newestModTime is the most recent modification anywhere under dir that is not @@ -837,6 +945,16 @@ func (c Checkout) Git(ctx context.Context, args ...string) (string, error) { return c.ws.git(ctx, c.Dir, args...) } +// SetPushURL gives this checkout its own origin push target. Fetch configuration +// remains shared with the mirror, while sibling checkouts keep their targets. +func (c Checkout) SetPushURL(ctx context.Context, url string) error { + if strings.TrimSpace(url) == "" { + return errors.New("push URL must not be empty") + } + _, err := c.ws.git(ctx, c.Dir, "config", "--worktree", "--replace-all", "remote.origin.pushurl", url) + return err +} + // gitDir runs git in dir ("" means the process's own directory) and returns its // trimmed stdout. Stderr is folded into the error, because "exit status 128" on // its own has never told anybody what went wrong. @@ -849,13 +967,50 @@ func Git(ctx context.Context, dir string, args ...string) (string, error) { return gitDir(ctx, dir, args...) } +var repositoryLocalGitEnv = map[string]bool{ + "GIT_ALTERNATE_OBJECT_DIRECTORIES": true, + "GIT_COMMON_DIR": true, + "GIT_CONFIG": true, + "GIT_CONFIG_COUNT": true, + "GIT_CONFIG_PARAMETERS": true, + "GIT_DIR": true, + "GIT_GRAFT_FILE": true, + "GIT_IMPLICIT_WORK_TREE": true, + "GIT_INDEX_FILE": true, + "GIT_NAMESPACE": true, + "GIT_NO_REPLACE_OBJECTS": true, + "GIT_OBJECT_DIRECTORY": true, + "GIT_PREFIX": true, + "GIT_REPLACE_REF_BASE": true, + "GIT_SHALLOW_FILE": true, + "GIT_WORK_TREE": true, +} + // gitEnv is gitDir with extra environment entries. func gitEnv(ctx context.Context, dir string, env []string, args ...string) (string, error) { - cmd := exec.CommandContext(ctx, "git", args...) + // A linked checkout can write executable settings into the shared mirror. + // Disable them even when no token is injected, and keep an invoking Git hook + // from redirecting commands away from dir through repository-local variables. + full := append([]string{ + "-c", "core.hooksPath=" + os.DevNull, + "-c", "core.fsmonitor=false", + }, args...) + cmd := exec.CommandContext(ctx, "git", full...) cmd.Dir = dir - if len(env) > 0 { - cmd.Env = append(os.Environ(), env...) + overrides := make(map[string]bool, len(env)) + for _, entry := range env { + name, _, _ := strings.Cut(entry, "=") + overrides[name] = true + } + parent := os.Environ() + cmd.Env = make([]string, 0, len(parent)+len(env)) + for _, entry := range parent { + name, _, _ := strings.Cut(entry, "=") + if !repositoryLocalGitEnv[name] && !overrides[name] { + cmd.Env = append(cmd.Env, entry) + } } + cmd.Env = append(cmd.Env, env...) var stderr strings.Builder cmd.Stderr = &stderr out, err := cmd.Output() diff --git a/internal/workspace/workspace_test.go b/internal/workspace/workspace_test.go index 2436cb8..e6b4668 100644 --- a/internal/workspace/workspace_test.go +++ b/internal/workspace/workspace_test.go @@ -6,6 +6,7 @@ import ( "os/exec" "path/filepath" "strings" + "sync" "testing" "time" ) @@ -215,6 +216,28 @@ func TestGitTokenTravelsInTheEnvironment(t *testing.T) { } } +func TestGitIgnoresAnInvokingRepositoriesEnvironment(t *testing.T) { + intended := t.TempDir() + other := t.TempDir() + originRepo(t, intended) + originRepo(t, other) + t.Setenv("GIT_DIR", filepath.Join(other, ".git")) + t.Setenv("GIT_WORK_TREE", other) + t.Setenv("GIT_INDEX_FILE", filepath.Join(other, ".git", "index")) + + got, err := gitDir(context.Background(), intended, "rev-parse", "--show-toplevel") + if err != nil { + t.Fatal(err) + } + want, err := filepath.EvalSymlinks(intended) + if err != nil { + t.Fatal(err) + } + if got != want { + t.Errorf("git selected %q from the invoking environment, want %q", got, want) + } +} + // A relative root would make `git worktree add` — which runs inside the mirror — // create the worktree under the mirror, while the path handed back points at a // directory that does not exist. @@ -425,27 +448,45 @@ func TestMirrorMigrationClearsThePushMirrorFlag(t *testing.T) { } } -// Linked worktrees share remote configuration with the mirror. A session may -// repoint origin for a fork push, but a base-repository refresh must neither -// trust nor rewrite that session's push destination. -func TestMirrorRefreshLeavesOriginURLAlone(t *testing.T) { +// Each checkout has its own push URL: a fork session must not repoint a sibling +// or the mirror, and a base-repository refresh must leave it alone. +func TestCheckoutPushURLsAreWorktreeLocal(t *testing.T) { base := t.TempDir() repo := "owner/thing" origin := filepath.Join(base, repo) - originRepo(t, origin) + sha := originRepo(t, origin) other := filepath.Join(base, "fork/thing") originRepo(t, other) t.Setenv("CRQ_REMOTE_BASE", base) ws := Workspace{Root: t.TempDir()} - ctx := context.Background() + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) - mirror, err := ws.Mirror(ctx, repo) + first, err := ws.Checkout(ctx, repo, 1, sha) if err != nil { t.Fatal(err) } - if _, err := gitDir(ctx, mirror, "remote", "set-url", "origin", other); err != nil { + second, err := ws.Checkout(ctx, repo, 2, sha) + if err != nil { t.Fatal(err) } + if err := second.SetPushURL(ctx, other); err != nil { + t.Fatal(err) + } + if got, err := first.Git(ctx, "remote", "get-url", "--push", "origin"); err != nil || got != ws.remoteURL(repo) { + t.Errorf("first push URL = %q err=%v, want base %q", got, err, ws.remoteURL(repo)) + } + if got, err := second.Git(ctx, "remote", "get-url", "--push", "origin"); err != nil || got != other { + t.Errorf("second push URL = %q err=%v, want fork %q", got, err, other) + } + mirror, err := ws.mirrorPath(repo) + if err != nil { + t.Fatal(err) + } + if got, err := gitDir(ctx, mirror, "config", "--local", "--get", "remote.origin.pushurl"); err == nil { + t.Errorf("shared push URL = %q, want none", got) + } + if err := os.WriteFile(filepath.Join(origin, "README.md"), []byte("base moved\n"), 0o644); err != nil { t.Fatal(err) } @@ -461,8 +502,8 @@ func TestMirrorRefreshLeavesOriginURLAlone(t *testing.T) { if _, err := ws.Mirror(ctx, repo); err != nil { t.Fatal(err) } - if got, err := gitDir(ctx, mirror, "remote", "get-url", "origin"); err != nil || got != other { - t.Errorf("origin URL = %q err=%v, want the session's %q left alone", got, err, other) + if got, err := second.Git(ctx, "remote", "get-url", "--push", "origin"); err != nil || got != other { + t.Errorf("fork push URL after refresh = %q err=%v, want %q", got, err, other) } if got, err := gitDir(ctx, mirror, "rev-parse", "refs/remotes/origin/main"); err != nil || got != want { t.Errorf("fetched main = %q err=%v, want base repository head %q", got, err, want) @@ -851,6 +892,80 @@ func TestMirrorKeepsAnEqualTipSessionBranchAfterMigration(t *testing.T) { } } +func TestConcurrentMigrationRechecksOwnershipAfterLock(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + origin := filepath.Join(base, repo) + sha := originRepo(t, origin) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + if _, err := gitDir(ctx, origin, "branch", "feature"); err != nil { + t.Fatal(err) + } + mirror, err := ws.mirrorPath(repo) + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(mirror), 0o700); err != nil { + t.Fatal(err) + } + if _, err := gitDir(ctx, "", "clone", "--mirror", "--quiet", ws.remoteURL(repo), mirror); err != nil { + t.Fatal(err) + } + + unlock, err := lockMirrorMigration(ctx, mirror) + if err != nil { + t.Fatal(err) + } + release := sync.OnceFunc(unlock) + defer release() + waiter := make(chan error, 1) + go func() { + _, err := ws.Mirror(ctx, repo) + waiter <- err + }() + select { + case err := <-waiter: + release() + t.Fatalf("concurrent refresh returned before the migration lock was released: %v", err) + case <-time.After(100 * time.Millisecond): + // The pre-lock checks are local config reads; still waiting proves this + // refresh reached the claim held above. + } + + // Complete the migration as the lock owner, then let a session reuse an + // equal-tip name before the waiting refresh acquires the claim. + if err := ws.migrateMirror(ctx, mirror); err != nil { + t.Fatal(err) + } + if err := ws.fetchMirror(ctx, mirror, repo); err != nil { + t.Fatal(err) + } + if err := ws.dropFetchedHeads(ctx, mirror); err != nil { + t.Fatal(err) + } + if err := ws.setConfig(ctx, mirror, fetchedHeadsMigratedKey, "true"); err != nil { + t.Fatal(err) + } + if _, err := gitDir(ctx, mirror, "update-ref", "refs/heads/feature", sha); err != nil { + t.Fatal(err) + } + release() + + select { + case err := <-waiter: + if err != nil { + t.Fatal(err) + } + case <-time.After(10 * time.Second): + t.Fatal("concurrent refresh did not finish after the migration lock was released") + } + if got, err := gitDir(ctx, mirror, "rev-parse", "refs/heads/feature"); err != nil || got != sha { + t.Errorf("session branch = %q err=%v, want preserved at %q", got, err, sha) + } +} + // A mirror with a second remote.origin.fetch refused a single-value write — // "cannot overwrite multiple values with a single value" — which would have // failed every later Mirror call for that repository for good. @@ -903,6 +1018,19 @@ func TestMirrorRetriesLegacyHeadCleanup(t *testing.T) { if _, err := gitDir(ctx, "", "clone", "--mirror", "--quiet", ws.remoteURL(repo), mirror); err != nil { t.Fatal(err) } + // Keep a loose copy behind the lock. Some Git versions remove only the + // packed copy before reporting the lock error, which is successful cleanup + // and must not make this retry test fail. + feature, err := gitDir(ctx, mirror, "rev-parse", "refs/heads/feature") + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(mirror, "refs", "heads"), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(mirror, "refs", "heads", "feature"), []byte(feature+"\n"), 0o600); err != nil { + t.Fatal(err) + } lock := filepath.Join(mirror, "refs", "heads", "feature.lock") if err := os.WriteFile(lock, nil, 0o600); err != nil { t.Fatal(err) @@ -1105,7 +1233,7 @@ func TestCheckoutGitRefreshesRotatedCredentials(t *testing.T) { } } -func TestMirrorPrunesDeletedTagsAndRefreshesMovedTags(t *testing.T) { +func TestMirrorTracksRemoteTagsWithoutChangingSessionTags(t *testing.T) { base := t.TempDir() repo := "owner/thing" origin := filepath.Join(base, repo) @@ -1123,6 +1251,16 @@ func TestMirrorPrunesDeletedTagsAndRefreshesMovedTags(t *testing.T) { if err != nil { t.Fatal(err) } + if got, err := gitDir(ctx, mirror, "rev-parse", "refs/crq/remotes/origin/tags/release"); err != nil || got != first { + t.Fatalf("tracked release tag = %q err=%v, want %q", got, err, first) + } + // These names now belong to sessions. A refresh must not infer ownership + // from whether an identically named tag exists on the remote. + for _, name := range []string{"deleted", "release"} { + if _, err := gitDir(ctx, mirror, "tag", name, first); err != nil { + t.Fatal(err) + } + } if _, err := gitDir(ctx, origin, "tag", "-d", "deleted"); err != nil { t.Fatal(err) @@ -1146,11 +1284,17 @@ func TestMirrorPrunesDeletedTagsAndRefreshesMovedTags(t *testing.T) { t.Fatal(err) } - if _, err := gitDir(ctx, mirror, "show-ref", "--verify", "--quiet", "refs/tags/deleted"); err == nil { - t.Error("deleted remote tag survived in the reused mirror") + if got, err := gitDir(ctx, mirror, "rev-parse", "refs/tags/deleted"); err != nil || got != first { + t.Errorf("session tag deleted = %q err=%v, want preserved at %q", got, err, first) } - if got, err := gitDir(ctx, mirror, "rev-parse", "refs/tags/release"); err != nil || got != second { - t.Errorf("release tag = %q err=%v, want moved tag %q (was %q)", got, err, second, first) + if got, err := gitDir(ctx, mirror, "rev-parse", "refs/tags/release"); err != nil || got != first { + t.Errorf("session release tag = %q err=%v, want preserved at %q", got, err, first) + } + if _, err := gitDir(ctx, mirror, "show-ref", "--verify", "--quiet", "refs/crq/remotes/origin/tags/deleted"); err == nil { + t.Error("deleted remote tag survived in the tracking namespace") + } + if got, err := gitDir(ctx, mirror, "rev-parse", "refs/crq/remotes/origin/tags/release"); err != nil || got != second { + t.Errorf("tracked release tag = %q err=%v, want moved tag %q", got, err, second) } } @@ -1228,6 +1372,44 @@ func TestCheckoutDisablesSharedExecutableConfiguration(t *testing.T) { } } +func TestTokenlessMirrorRefreshDisablesSharedHooks(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + origin := filepath.Join(base, repo) + originRepo(t, origin) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + mirror, err := ws.Mirror(ctx, repo) + if err != nil { + t.Fatal(err) + } + hooks := t.TempDir() + marker := filepath.Join(t.TempDir(), "reference-transaction-ran") + hook := filepath.Join(hooks, "reference-transaction") + if err := os.WriteFile(hook, []byte("#!/bin/sh\ntouch \""+marker+"\"\n"), 0o700); err != nil { + t.Fatal(err) + } + if _, err := gitDir(ctx, mirror, "config", "core.hooksPath", hooks); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(origin, "README.md"), []byte("two\n"), 0o644); err != nil { + t.Fatal(err) + } + for _, args := range [][]string{{"add", "README.md"}, {"commit", "-m", "second"}} { + if _, err := gitDir(ctx, origin, args...); err != nil { + t.Fatal(err) + } + } + + if _, err := ws.Mirror(ctx, repo); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(marker); !os.IsNotExist(err) { + t.Errorf("shared reference-transaction hook ran during refresh: %v", err) + } +} + func TestFreshHeartbeatAvoidsDeepPruningDecision(t *testing.T) { dir := t.TempDir() old := time.Now().Add(-2 * staleWorkAge) @@ -1247,6 +1429,29 @@ func TestFreshHeartbeatAvoidsDeepPruningDecision(t *testing.T) { } } +// A backward clock correction can leave the live root heartbeat ahead of now +// while older files remain behind it. The future heartbeat is indeterminate, +// not evidence that the checkout is abandoned. +func TestFutureRootHeartbeatAvoidsDeepPruningDecision(t *testing.T) { + dir := t.TempDir() + old := time.Now().Add(-2 * staleWorkAge) + file := filepath.Join(dir, "generated.bin") + if err := os.WriteFile(file, []byte("old tree stand-in"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Chtimes(file, old, old); err != nil { + t.Fatal(err) + } + now := time.Now() + future := now.Add(time.Hour) + if err := os.Chtimes(dir, future, future); err != nil { + t.Fatal(err) + } + if !rootHeartbeatFresh(dir, now) { + t.Fatal("future checkout heartbeat was discarded before the deep scan") + } +} + // A worktree is made for somebody else to work in, and that somebody pushes with // a plain `git push`. crq's own commands pass the credential helper with -c, // which lasts one command, so the mirror has to carry it for the ones this