From 385cf404f96c871f8344c04d0642ee49a2471f6e Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Mon, 9 Nov 2026 15:47:05 -0500 Subject: [PATCH 1/3] fix(workbench): keep git checkouts within Git for Windows' GIT_DIR limit Reviewer clones nested about 115 characters deep in the run artifact tree, so their .git paths passed the 220 characters Git for Windows accepts for GIT_DIR. The transport clone from #649 hands GIT_DIR to index-pack, and reviews failed with "'$GIT_DIR' too big" before any reviewer ran. Move the workbench repo and reviewer clones to a flat /ws//r, keyed by a hash of run directory, role, and agent ID, with an owner record that data prune uses to sweep checkouts whose run is gone. Set core.longpaths on checkouts, and fail early on Windows with the fix when a .git path would still be too long. --- README.md | 9 +- docs/checkout-native-review-contract.md | 52 ++++-- docs/review-guidance.md | 17 +- internal/datalifecycle/datalifecycle.go | 101 +++++++++++- internal/datalifecycle/workspaces_test.go | 132 ++++++++++++++++ internal/pipeline/pipeline.go | 10 +- internal/pipeline/pipeline_test.go | 17 +- internal/pipeline/workbench_cleanup_test.go | 21 ++- .../pipeline/workbench_integration_test.go | 2 +- internal/runartifact/runartifact.go | 12 +- internal/runartifact/workspaces.go | 144 +++++++++++++++++ internal/runartifact/workspaces_test.go | 148 ++++++++++++++++++ internal/statepaths/statepaths.go | 10 ++ internal/workbench/git_dir_length_test.go | 73 +++++++++ internal/workbench/workbench.go | 62 +++++++- 15 files changed, 764 insertions(+), 46 deletions(-) create mode 100644 internal/datalifecycle/workspaces_test.go create mode 100644 internal/runartifact/workspaces.go create mode 100644 internal/runartifact/workspaces_test.go create mode 100644 internal/workbench/git_dir_length_test.go diff --git a/README.md b/README.md index 4049d749..79837386 100644 --- a/README.md +++ b/README.md @@ -692,7 +692,7 @@ Supported values: | `review_policy.major_event` | `comment`, `request_changes`. Sets the review event when the highest severity is major. Blocking findings always request changes, a major finding never approves, and the rollup model's proposed event cannot override either rule. Under `request_changes`, the rollup model may soften a major-only review to a comment. | | `review_policy.resolve_threads` | `auto`, `never` | | `data.retention.enforcement` | `at_write` applies review-time pruning before each `cr review`; `manual_only` disables review-time pruning and leaves `cr data prune` as the explicit maintenance path. Review-time pruning runs only when no other cr instance is mid-run (it requires the data-root active-runs lock exclusively) and is skipped, not queued, on contention. | -| `data.keep_workbench` | `true` retains the run checkout under `workbench/` after a successful run. Failed and errored runs always retain it. | +| `data.keep_workbench` | `true` retains the run workbench (`workbench/` and its checkout under the data root's `ws/`) after a successful run. Failed and errored runs always retain it. | `subscription` LLM auth means the adapter owns its own credentials, such as a logged-in CLI or local runtime. `api_key` LLM auth requires `llm.credential` @@ -1716,8 +1716,11 @@ command remains the safe preview path for manual maintenance, and no-selector `cr data prune` uses its built-in live 90-day and dry-run 7-day windows independent from profile config. -Successful runs delete their run checkout (`workbench/`) once the run reaches a -successful terminal state. Failed and errored runs retain it for inspection, and +Successful runs delete their run checkout (`workbench/`, plus the git checkout +kept under the data root's `ws/` directory) once the run reaches a successful +terminal state. Git checkouts live in `ws/` rather than inside the run directory +so their `.git` paths fit Git for Windows' 220-character limit; `cr data prune` +also removes any checkout whose run is gone. Failed and errored runs retain it for inspection, and `data.keep_workbench: true` retains it for successful runs too. `cr review --keep-workbench` overrides the config for one invocation; an explicit `--keep-workbench=false` forces deletion even when `data.keep_workbench: true`. diff --git a/docs/checkout-native-review-contract.md b/docs/checkout-native-review-contract.md index ca479b1d..c27986bb 100644 --- a/docs/checkout-native-review-contract.md +++ b/docs/checkout-native-review-contract.md @@ -43,9 +43,10 @@ dossier assembly, and the dossier is assembled before orchestrator selection. ## Artifact Model -All checkout-native review artifacts are owned by the review run and must live +All checkout-native review artifacts are owned by the review run and live under the run artifact directory so existing retention, `cr data prune`, and -`cr data purge` remove them automatically. +`cr data purge` remove them automatically. The one exception is git checkouts, +which live in the flat workspaces root described below. The target layout is: @@ -63,12 +64,6 @@ runs// final/ index.json workbench/ - repo/ - reviewers/ - / - repo/ - -coverage-repair/ - repo/ scratch/ / cache/ @@ -77,8 +72,35 @@ runs// cache/ tmp/ metadata.json + +ws// + r/ git checkout + owner.json owning run directory, role, and agent ID ``` +Git checkouts (the run workbench repo and each reviewer clone) live under +`/ws//r` beside `runs/`, not inside the run directory. Git for +Windows refuses a `GIT_DIR` longer than 220 characters (its `PATH_MAX` of 260 +minus the 40 that `setup.c` reserves), and a transport clone hands `GIT_DIR` to +its `index-pack` child, so a checkout nested in the run tree failed with +`'$GIT_DIR' too big` for longer repository or reviewer names. A workspace's +`.git` path is the data root plus a fixed 23 characters, whatever the PR or +reviewer. `` is a 12-character hash of the run directory, role +(`workbench` or `reviewer`), and agent ID, so a resumed run finds the same +workspaces and no two runs or reviewers share one. On Windows, workspace +preparation checks the `.git` length before running git and fails with the fix +when the data root itself is too long. Checkouts set `core.longpaths=true` so +Git for Windows can also check out files whose full path passes 260 +characters; other platforms ignore the setting. + +`cr data prune` removes a workspace whose `owner.json` names a run directory +that no longer exists (including runs that prune itself deletes), and one with +no readable owner record once it is older than the orphan grace period. +`cr data purge` removes the whole data root, workspaces included. Run +directories outside the standard `/runs/.../run-` layout, such as +caller-owned benchmark directories, keep their checkouts under `workbench/` as +before. + Notes: - `llm-tasks/` keeps the existing durable structured-task artifacts. @@ -87,8 +109,8 @@ Notes: - `dossier/summary/` holds durable normalized discussion artifacts. - `dossier/final/` holds the reviewer-facing dossier files used by the orchestrator and specialists. -- `workbench/repo/` is a clean pinned checkout at the PR head SHA, carrying - `refs/heads/cr-review-head` at that SHA. The ref is load-bearing: the +- The workbench checkout (`ws//r`, role `workbench`) is a clean pinned + checkout at the PR head SHA, carrying `refs/heads/cr-review-head` at that SHA. The ref is load-bearing: the per-reviewer workspace is created with `git clone` from this directory, and git does not treat a directory without `refs/` as a repository. Only the head is given a ref, so the base commit is not transferred into reviewer @@ -96,15 +118,15 @@ Notes: provider-generated `diff.patch` and nothing in the pipeline resolves the base SHA inside the workspace. That is a decision, not an oversight -- add `refs/heads/cr-review-base` if a reviewer ever needs `git log base..HEAD`. -- `workbench/reviewers//repo/` is a disposable reviewer checkout. - Clone it through Git transport (`--no-local`) rather than copying the +- Each reviewer's `ws//r` (role `reviewer`) is a disposable reviewer + checkout. Clone it through Git transport (`--no-local`) rather than copying the canonical object directory: background repacking can replace pack files during a local-copy clone. Each checkout has its own object database and receives only history reachable from the canonical refs. - `workbench/scratch//` holds reviewer-owned scratch, temp, and cache roots. - A coverage-repair pass runs under the derived identity - `-coverage-repair`, so a repaired run also has that reviewer + `-coverage-repair`, so a repaired run also has its own reviewer checkout, that scratch root, and its own encoded agent log. Workspace preparation clears the directory it is given, so the repair must never reuse the primary reviewer's identity. @@ -117,8 +139,8 @@ Notes: The workbench is run-owned, not cache-owned. Shared clone or fetch caches are a possible future optimization but are not part of the correctness contract. -A successful run removes its `workbench/` tree when it reaches a successful -terminal state, after rollup and plan build, so retention no longer pins a full +A successful run removes its `workbench/` tree and its workbench checkout when +it reaches a successful terminal state, after rollup and plan build, so retention no longer pins a full checkout per run. A live run reaches that state when its outbox post succeeds; failed, aborted, and incomplete runs retain the workbench for inspection, and `data.keep_workbench: true` opts a run back into retention on success. The diff --git a/docs/review-guidance.md b/docs/review-guidance.md index f8992e18..ab7ae479 100644 --- a/docs/review-guidance.md +++ b/docs/review-guidance.md @@ -108,16 +108,21 @@ review run artifact root: runs// dossier/ workbench/ +ws// + r/ + owner.json ``` -Because those directories live under the run artifact root, existing data -lifecycle commands clean them up automatically: +Git checkouts live in `ws/` rather than the run directory so their `.git` paths +stay within Git for Windows' 220-character limit; each records its owning run. +Data lifecycle commands clean both up: -- `cr data prune` removes dossier and workbench directories for pruned runs -- `cr data purge` removes the entire data root, including dossier and workbench - artifacts +- `cr data prune` removes dossier and workbench directories for pruned runs, + and any workspace whose owning run directory is gone +- `cr data purge` removes the entire data root, including dossier, workbench, + and workspace artifacts -A successful run also deletes its own `workbench/` directory once the run +A successful run also deletes its own `workbench/` directory and checkout once the run reaches a successful terminal state, so dossier data is the durable artifact that remains. Failed and errored runs always retain the workbench for inspection, and `data.keep_workbench: true` opts successful runs back into retention. diff --git a/internal/datalifecycle/datalifecycle.go b/internal/datalifecycle/datalifecycle.go index 12746f0a..e954285a 100644 --- a/internal/datalifecycle/datalifecycle.go +++ b/internal/datalifecycle/datalifecycle.go @@ -13,6 +13,7 @@ import ( "time" "github.com/open-cli-collective/codereview-cli/internal/ledger" + "github.com/open-cli-collective/codereview-cli/internal/runartifact" "github.com/open-cli-collective/codereview-cli/internal/runlock" "github.com/open-cli-collective/codereview-cli/internal/statepaths" ) @@ -135,6 +136,11 @@ func Show(ctx context.Context, opts Options) (Stats, error) { if err != nil { return Stats{}, err } + workspaceOrphans, err := orphanWorkspaces(opts.Layout, opts.now().Add(-OrphanGrace), nil) + if err != nil { + return Stats{}, err + } + orphanItems = append(orphanItems, workspaceOrphans...) stats := Stats{ DataRoot: opts.Layout.DataRoot, LedgerPath: opts.Layout.LedgerDB(), @@ -165,6 +171,11 @@ func Show(ctx context.Context, opts Options) (Stats, error) { if err != nil { return Stats{}, err } + workspaceBytes, err := dirBytes(opts.Layout.WorkspacesRoot()) + if err != nil { + return Stats{}, err + } + stats.ArtifactBytes += workspaceBytes for _, orphan := range orphanItems { stats.OrphanCount++ stats.OrphanBytes += orphan.Bytes @@ -232,8 +243,21 @@ func Prune(ctx context.Context, opts Options, prune PruneOptions) (PruneResult, orphanSpan.End(err) return result, err } + // Workspaces whose run this prune would delete are reported with it. + removing := map[string]bool{} + for _, run := range selected { + removing[filepath.Clean(run.ArtifactPath)] = true + } + for _, orphan := range orphanItems { + removing[filepath.Clean(orphan.Path)] = true + } + workspaceOrphans, err := orphanWorkspaces(opts.Layout, opts.now().Add(-OrphanGrace), removing) + if err != nil { + orphanSpan.End(err) + return result, err + } orphanSpan.End(nil) - result.OrphansRemoved = orphanItems + result.OrphansRemoved = append(orphanItems, workspaceOrphans...) return result, nil } @@ -275,6 +299,20 @@ func Prune(ctx context.Context, opts Options, prune PruneOptions) (PruneResult, } result.OrphansRemoved = append(result.OrphansRemoved, orphan) } + // Run directories are gone by now, so this also catches the workspaces of + // every run deleted above. + workspaceOrphans, err := orphanWorkspaces(opts.Layout, opts.now().Add(-OrphanGrace), nil) + if err != nil { + orphanSpan.End(err) + return result, err + } + for _, orphan := range workspaceOrphans { + if err := opts.removeAll()(orphan.Path); err != nil && !errors.Is(err, fs.ErrNotExist) { + result.Warnings = append(result.Warnings, fmt.Sprintf("failed to remove orphan workspace at %s: %v", orphan.Path, err)) + continue + } + result.OrphansRemoved = append(result.OrphansRemoved, orphan) + } orphanSpan.End(nil) _ = removeEmptyParents(runsRoot(opts.Layout)) return result, nil @@ -462,6 +500,67 @@ func orphanItems(layout statepaths.Layout, runs []ledger.Run, graceCutoff time.T return items, nil } +// orphanWorkspaces lists git workspaces whose owning run directory no longer +// exists or is in removing. A workspace without a readable owner record (a run +// that stopped between creating the directory and recording its owner) is +// swept once it is older than graceCutoff. Symlinked entries are never +// followed or removed. +func orphanWorkspaces(layout statepaths.Layout, graceCutoff time.Time, removing map[string]bool) ([]OrphanItem, error) { + root := layout.WorkspacesRoot() + if err := rejectSymlink(root); err != nil { + return nil, err + } + entries, err := os.ReadDir(root) + if errors.Is(err, fs.ErrNotExist) { + return nil, nil + } + if err != nil { + return nil, err + } + var items []OrphanItem + for _, entry := range entries { + if !entry.IsDir() || entry.Type()&fs.ModeSymlink != 0 { + continue + } + path := filepath.Join(root, entry.Name()) + orphaned, err := workspaceOrphaned(path, entry, graceCutoff, removing) + if err != nil { + return nil, err + } + if !orphaned { + continue + } + bytes, err := dirBytes(path) + if err != nil { + return nil, err + } + items = append(items, OrphanItem{Path: path, Bytes: bytes}) + } + sort.Slice(items, func(i, j int) bool { return items[i].Path < items[j].Path }) + return items, nil +} + +func workspaceOrphaned(path string, entry fs.DirEntry, graceCutoff time.Time, removing map[string]bool) (bool, error) { + owner, err := runartifact.ReadWorkspaceOwner(path) + if err != nil { + info, infoErr := entry.Info() + if infoErr != nil { + return false, infoErr + } + return !info.ModTime().After(graceCutoff), nil + } + runDir := filepath.Clean(owner.RunDir) + if removing[runDir] { + return true, nil + } + if _, err := os.Stat(runDir); errors.Is(err, fs.ErrNotExist) { + return true, nil + } else if err != nil { + return false, err + } + return false, nil +} + func referencedArtifacts(layout statepaths.Layout, runs []ledger.Run) (map[string]bool, error) { referenced := map[string]bool{} for _, run := range runs { diff --git a/internal/datalifecycle/workspaces_test.go b/internal/datalifecycle/workspaces_test.go new file mode 100644 index 00000000..cd04a077 --- /dev/null +++ b/internal/datalifecycle/workspaces_test.go @@ -0,0 +1,132 @@ +package datalifecycle + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "reflect" + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/ledger" + "github.com/open-cli-collective/codereview-cli/internal/runartifact" +) + +// writeWorkspace creates a workspace in the layout's workspaces root, owned by +// runDir when it is non-empty, and returns its path. +func writeWorkspace(t *testing.T, root, name, runDir string) string { + t.Helper() + dir := filepath.Join(root, name) + writeFile(t, filepath.Join(dir, "r", "main.go"), "package main") + if runDir != "" { + data, err := json.Marshal(runartifact.WorkspaceOwner{SchemaVersion: 1, RunDir: runDir, Role: runartifact.WorkspaceRoleReviewer, AgentID: "harness:alpha"}) + if err != nil { + t.Fatalf("Marshal: %v", err) + } + writeFile(t, filepath.Join(dir, runartifact.WorkspaceOwnerFile), string(data)) + } + return dir +} + +func TestPruneSweepsWorkspacesWhoseRunIsGone(t *testing.T) { + layout := testLayout(t) + root := layout.WorkspacesRoot() + kept := testRun(layout, "kept", ledger.PostModeLive, testNow()) + expired := testRun(layout, "expired", ledger.PostModeDryRun, testNow().Add(-2*DryRunRetention)) + writeFile(t, filepath.Join(kept.ArtifactPath, "rollup.md"), "keep") + writeFile(t, filepath.Join(expired.ArtifactPath, "rollup.md"), "expire") + + keptWorkspace := writeWorkspace(t, root, "aaaaaaaaaaaa", kept.ArtifactPath) + expiredWorkspace := writeWorkspace(t, root, "bbbbbbbbbbbb", expired.ArtifactPath) + goneWorkspace := writeWorkspace(t, root, "cccccccccccc", filepath.Join(layout.DataRoot, "runs", "deleted-long-ago")) + staleUnowned := writeWorkspace(t, root, "dddddddddddd", "") + backdate(t, staleUnowned, testNow().Add(-2*OrphanGrace)) + freshUnowned := writeWorkspace(t, root, "eeeeeeeeeeee", "") + backdate(t, freshUnowned, testNow().Add(-OrphanGrace/2)) + store := &fakeStore{runs: []ledger.Run{kept, expired}} + opts := Options{Layout: layout, Store: store, Now: testNow} + + dry, err := Prune(context.Background(), opts, PruneOptions{DryRun: true}) + if err != nil { + t.Fatalf("Prune dry run: %v", err) + } + want := []string{expiredWorkspace, goneWorkspace, staleUnowned} + if got := orphanPaths(dry.OrphansRemoved); !reflect.DeepEqual(got, want) { + t.Fatalf("dry-run orphans = %#v, want the workspaces of the expiring and missing runs plus the stale unowned one %#v", got, want) + } + for _, path := range want { + if _, err := os.Stat(path); err != nil { + t.Fatalf("dry run removed %s: %v", path, err) + } + } + + result, err := Prune(context.Background(), opts, PruneOptions{}) + if err != nil { + t.Fatalf("Prune: %v", err) + } + if got := runItemIDs(result.DeletedRuns); !reflect.DeepEqual(got, []string{"expired"}) { + t.Fatalf("deleted runs = %#v, want expired", got) + } + if got := orphanPaths(result.OrphansRemoved); !reflect.DeepEqual(got, want) { + t.Fatalf("orphans removed = %#v, want %#v", got, want) + } + for _, path := range want { + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Fatalf("%s stat err = %v, want removed", path, err) + } + } + for _, path := range []string{keptWorkspace, freshUnowned} { + if _, err := os.Stat(path); err != nil { + t.Fatalf("%s removed, want kept: %v", path, err) + } + } +} + +func TestPruneNeverFollowsSymlinkedWorkspaces(t *testing.T) { + layout := testLayout(t) + target := writeWorkspace(t, t.TempDir(), "outside", filepath.Join(layout.DataRoot, "runs", "gone")) + if err := os.MkdirAll(layout.WorkspacesRoot(), 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.Symlink(target, filepath.Join(layout.WorkspacesRoot(), "linked")); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + result, err := Prune(context.Background(), Options{Layout: layout, Store: &fakeStore{}, Now: testNow}, PruneOptions{}) + if err != nil { + t.Fatalf("Prune: %v", err) + } + if len(result.OrphansRemoved) != 0 { + t.Fatalf("orphans removed = %#v, want the symlinked workspace left alone", result.OrphansRemoved) + } + if _, err := os.Stat(filepath.Join(target, "r", "main.go")); err != nil { + t.Fatalf("symlink target was touched: %v", err) + } +} + +func TestShowCountsWorkspaceBytesAndOrphans(t *testing.T) { + layout := testLayout(t) + live := testRun(layout, "live", ledger.PostModeLive, testNow()) + writeFile(t, filepath.Join(live.ArtifactPath, "rollup.md"), "12345") + owned := writeWorkspace(t, layout.WorkspacesRoot(), "aaaaaaaaaaaa", live.ArtifactPath) + orphan := writeWorkspace(t, layout.WorkspacesRoot(), "bbbbbbbbbbbb", filepath.Join(layout.DataRoot, "runs", "gone")) + ownedBytes, err := dirBytes(owned) + if err != nil { + t.Fatalf("dirBytes: %v", err) + } + orphanBytes, err := dirBytes(orphan) + if err != nil { + t.Fatalf("dirBytes: %v", err) + } + + stats, err := Show(context.Background(), Options{Layout: layout, Store: &fakeStore{runs: []ledger.Run{live}}, Now: testNow}) + if err != nil { + t.Fatalf("Show: %v", err) + } + if stats.OrphanCount != 1 || stats.OrphanBytes != orphanBytes { + t.Fatalf("orphans = %d (%d bytes), want the one workspace without a run (%d bytes)", stats.OrphanCount, stats.OrphanBytes, orphanBytes) + } + if want := 5 + ownedBytes + orphanBytes; stats.ArtifactBytes != want { + t.Fatalf("ArtifactBytes = %d, want run artifacts plus workspaces = %d", stats.ArtifactBytes, want) + } +} diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 311596fd..8e8fcf60 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -3802,7 +3802,15 @@ func RemoveWorkbench(artifactDir string, keepWorkbench bool) error { if keepWorkbench || strings.TrimSpace(artifactDir) == "" { return nil } - return os.RemoveAll(ArtifactPathsFromDir(artifactDir).WorkbenchDir) + paths := ArtifactPathsFromDir(artifactDir) + // The workbench checkout lives in the flat workspaces root, outside + // WorkbenchDir, so it is removed on its own. + if workspace := paths.WorkbenchWorkspace(); workspace != "" { + if err := os.RemoveAll(workspace); err != nil { + return err + } + } + return os.RemoveAll(paths.WorkbenchDir) } // tryPruneRetention runs automatic retention through the guarded entry diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 40447733..c3903737 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -1535,10 +1535,8 @@ func TestDryRunCoverageRepairWorkspaceFailurePreservesPrimaryReview(t *testing.T ResolveRepoRoot: func(context.Context) (string, error) { return repoDir, nil }, GitCommand: func(gitCtx context.Context, dir string, args ...string) ([]byte, error) { // Fail only the repair pass's clone, as a transient git or disk error would. - for _, arg := range args { - if strings.Contains(arg, workbench.ReviewerWorkspaceSegment(reviewerCoverageRepairIdentity("harness:reviewer"))) { - return nil, errors.New("disk full") - } + if isReviewerCloneForTest(args, reviewerCoverageRepairIdentity("harness:reviewer")) { + return nil, errors.New("disk full") } return gitCommand(gitCtx, dir, args...) }, @@ -1766,10 +1764,8 @@ func TestRunReviewerRecordsPrimarySessionOnEveryCoverageRepairPath(t *testing.T) if tc.failRepairClone { gitCommand := opts.GitCommand opts.GitCommand = func(gitCtx context.Context, dir string, args ...string) ([]byte, error) { - for _, arg := range args { - if strings.Contains(arg, workbench.ReviewerWorkspaceSegment(reviewerCoverageRepairIdentity("harness:reviewer"))) { - return nil, errors.New("disk full") - } + if isReviewerCloneForTest(args, reviewerCoverageRepairIdentity("harness:reviewer")) { + return nil, errors.New("disk full") } return gitCommand(gitCtx, dir, args...) } @@ -2052,7 +2048,7 @@ func TestDryRunWithPinnedReviewSHAsUsesCompareDiffAndPinnedFileRefs(t *testing.T t.Fatalf("reviewer request = %#v, want reviewer workspace", requests[1]) } workspace := requests[1].ReviewerWorkspace - if !strings.Contains(workspace.RepoDir, filepath.Join("workbench", "reviewers")) || + if !strings.HasPrefix(workspace.RepoDir, result.Artifacts.WorkspacesRoot+string(filepath.Separator)) || !strings.HasPrefix(workspace.ScratchDir, result.Artifacts.WorkbenchScratch+string(filepath.Separator)) || workspace.DiffPath != result.Artifacts.DiffPatch || workspace.MaxToolOutputBytes != 32*1024 { @@ -3247,7 +3243,8 @@ func TestDryRunReviewerFailureIsolation(t *testing.T) { } for _, agentID := range []string{"harness:alpha", "harness:beta", "harness:gamma"} { encoded := workbench.ReviewerWorkspaceSegment(agentID) - if _, err := os.Stat(filepath.Join(result.Artifacts.WorkbenchDir, "reviewers", encoded)); !errors.Is(err, os.ErrNotExist) { + workspaceRoot, _ := result.Artifacts.ReviewerWorkspace(agentID, encoded) + if _, err := os.Stat(workspaceRoot); !errors.Is(err, os.ErrNotExist) { t.Fatalf("reviewer workspace %s stat err = %v, want cleaned", agentID, err) } if _, err := os.Stat(filepath.Join(result.Artifacts.WorkbenchScratch, encoded)); !errors.Is(err, os.ErrNotExist) { diff --git a/internal/pipeline/workbench_cleanup_test.go b/internal/pipeline/workbench_cleanup_test.go index 43d17fe4..3c911f23 100644 --- a/internal/pipeline/workbench_cleanup_test.go +++ b/internal/pipeline/workbench_cleanup_test.go @@ -10,6 +10,7 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/gitprovider" "github.com/open-cli-collective/codereview-cli/internal/ledger" "github.com/open-cli-collective/codereview-cli/internal/llm" + "github.com/open-cli-collective/codereview-cli/internal/runartifact" "github.com/open-cli-collective/codereview-cli/internal/runlifecycle" "github.com/open-cli-collective/codereview-cli/internal/statepaths" ) @@ -76,6 +77,12 @@ func TestDryRunRemovesWorkbenchAfterSuccess(t *testing.T) { if _, err := os.Stat(artifacts.WorkbenchDir); !errors.Is(err, os.ErrNotExist) { t.Fatalf("workbench stat err = %v, want removed after success", err) } + if artifacts.WorkbenchWorkspace() == "" { + t.Fatal("WorkbenchWorkspace is empty, want the checkout in the workspaces root") + } + if _, err := os.Stat(artifacts.WorkbenchWorkspace()); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("workbench workspace stat err = %v, want removed after success", err) + } for _, path := range []string{artifacts.FindingsJSON, artifacts.RollupMarkdown} { if _, err := os.Stat(path); err != nil { t.Fatalf("artifact %s missing after workbench cleanup: %v", path, err) @@ -92,7 +99,7 @@ func TestDryRunRetainsWorkbenchAfterFailure(t *testing.T) { if !errors.Is(err, providerErr) { t.Fatalf("DryRun error = %v, want selection provider failure", err) } - if _, err := os.Stat(filepath.Join(artifacts.WorkbenchDir, "repo")); err != nil { + if _, err := os.Stat(artifacts.WorkbenchRepoDir); err != nil { t.Fatalf("workbench missing after failed run: %v", err) } } @@ -111,7 +118,7 @@ func TestDryRunRetainsWorkbenchWhenKeepWorkbenchEnabled(t *testing.T) { if err != nil { t.Fatalf("DryRun: %v", err) } - if _, err := os.Stat(filepath.Join(artifacts.WorkbenchDir, "repo")); err != nil { + if _, err := os.Stat(artifacts.WorkbenchRepoDir); err != nil { t.Fatalf("workbench missing with KeepWorkbench enabled: %v", err) } } @@ -164,3 +171,13 @@ func TestLiveRetainsWorkbenchUntilPostCompletes(t *testing.T) { t.Fatalf("workbench removed before the live post: %v", err) } } + +// isReviewerCloneForTest reports whether args clone the reviewer workspace for +// agentID, identified by the owner record written beside the checkout. +func isReviewerCloneForTest(args []string, agentID string) bool { + if len(args) == 0 || args[0] != "clone" { + return false + } + owner, err := runartifact.ReadWorkspaceOwner(filepath.Dir(args[len(args)-1])) + return err == nil && owner.Role == runartifact.WorkspaceRoleReviewer && owner.AgentID == agentID +} diff --git a/internal/pipeline/workbench_integration_test.go b/internal/pipeline/workbench_integration_test.go index 977964fe..37adf182 100644 --- a/internal/pipeline/workbench_integration_test.go +++ b/internal/pipeline/workbench_integration_test.go @@ -345,7 +345,7 @@ func TestDryRunReviewerWorkspaceAvoidsContextStuffingBudgetFailures(t *testing.T t.Fatalf("reviewer request = %#v, want reviewer workspace", requests[1]) } workspace := requests[1].ReviewerWorkspace - if !strings.Contains(workspace.RepoDir, filepath.Join("workbench", "reviewers")) || + if !strings.HasPrefix(workspace.RepoDir, result.Artifacts.WorkspacesRoot+string(filepath.Separator)) || !strings.HasPrefix(workspace.ScratchDir, result.Artifacts.WorkbenchScratch+string(filepath.Separator)) || workspace.MaxToolOutputBytes != 32*1024 { t.Fatalf("reviewer workspace request = %#v, want disposable repo/scratch with default cap", workspace) diff --git a/internal/runartifact/runartifact.go b/internal/runartifact/runartifact.go index f55045cf..bbb08fc1 100644 --- a/internal/runartifact/runartifact.go +++ b/internal/runartifact/runartifact.go @@ -52,6 +52,10 @@ type Paths struct { RelocationsJSON string `json:"relocations_json"` SymlinkMetadataJSON string `json:"symlink_metadata_json"` CoverageJSON string `json:"coverage_json"` + // WorkspacesRoot is the flat directory holding this run's git workspaces, + // or "" when the run directory is not in the standard data-root layout and + // workspaces stay under WorkbenchDir. + WorkspacesRoot string `json:"workspaces_root,omitempty"` } // ForRun returns the artifact paths for a generated run ID. @@ -70,6 +74,11 @@ func ForRun(layout statepaths.Layout, ref gitprovider.PRRef, pr gitprovider.PR, // FromDir returns the artifact path set rooted at dir. func FromDir(dir string) Paths { + workspacesRoot := workspacesRootForRunDir(dir) + workbenchRepoDir := filepath.Join(dir, "workbench", "repo") + if workspacesRoot != "" { + workbenchRepoDir = filepath.Join(workspaceDir(workspacesRoot, dir, WorkspaceRoleWorkbench, ""), workspaceRepoName) + } return Paths{ Dir: dir, DiffPatch: filepath.Join(dir, "diff.patch"), @@ -81,11 +90,12 @@ func FromDir(dir string) Paths { LLMTasksDir: filepath.Join(dir, "llm-tasks"), DossierDir: filepath.Join(dir, "dossier"), WorkbenchDir: filepath.Join(dir, "workbench"), - WorkbenchRepoDir: filepath.Join(dir, "workbench", "repo"), + WorkbenchRepoDir: workbenchRepoDir, WorkbenchScratch: filepath.Join(dir, "workbench", "scratch"), RelocationsJSON: filepath.Join(dir, "relocations.json"), SymlinkMetadataJSON: filepath.Join(dir, "symlink-metadata.json"), CoverageJSON: filepath.Join(dir, "coverage.json"), + WorkspacesRoot: workspacesRoot, } } diff --git a/internal/runartifact/workspaces.go b/internal/runartifact/workspaces.go new file mode 100644 index 00000000..b5d6d35c --- /dev/null +++ b/internal/runartifact/workspaces.go @@ -0,0 +1,144 @@ +package runartifact + +import ( + "crypto/sha256" + "encoding/binary" + "encoding/hex" + "encoding/json" + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/open-cli-collective/codereview-cli/internal/fsatomic" + "github.com/open-cli-collective/codereview-cli/internal/statepaths" +) + +// Git workspaces (the run workbench repo and each reviewer clone) live in a +// flat directory beside the run tree rather than inside it: +// +// /ws//r the git checkout +// /ws//owner.json which run and role own it +// +// The run tree nests five levels under /runs and repeats the PR's host, +// owner and repo, so a checkout inside it leaves little room under Git for +// Windows' 220-character GIT_DIR limit. A workspace's .git path is instead the +// data root plus a fixed 23 characters, whatever the repository, PR or +// reviewer. The key hashes the run directory, role and agent ID, so it is +// stable across resumes of one run and distinct between runs and reviewers. +const ( + // WorkspaceOwnerFile names the ownership record beside each checkout. + WorkspaceOwnerFile = "owner.json" + // WorkspaceRoleWorkbench marks the run's canonical workbench checkout. + WorkspaceRoleWorkbench = "workbench" + // WorkspaceRoleReviewer marks one reviewer's disposable checkout. + WorkspaceRoleReviewer = "reviewer" + + workspaceOwnerSchema = 1 + workspaceRepoName = "r" + workspaceKeyLength = 12 + // runDirDepth is how far a run directory sits below /runs: + // runs/////run-. + runDirDepth = 5 +) + +// WorkspaceOwner records which run owns a workspace, so data lifecycle commands +// can remove workspaces whose run artifacts are gone. +type WorkspaceOwner struct { + SchemaVersion int `json:"schema_version"` + RunDir string `json:"run_dir"` + Role string `json:"role"` + AgentID string `json:"agent_id,omitempty"` +} + +// WorkbenchWorkspace returns the workspace directory holding the run's +// workbench repo, or "" when the repo lives inside WorkbenchDir. +func (p Paths) WorkbenchWorkspace() string { + if p.WorkspacesRoot == "" { + return "" + } + return filepath.Dir(p.WorkbenchRepoDir) +} + +// ReviewerWorkspace returns the workspace root and checkout directory for one +// reviewer. legacySegment names the reviewer's directory when the run has no +// workspaces root and the checkout stays under WorkbenchDir. +func (p Paths) ReviewerWorkspace(agentID, legacySegment string) (root, repo string) { + if p.WorkspacesRoot == "" { + root = filepath.Join(p.WorkbenchDir, "reviewers", legacySegment) + return root, filepath.Join(root, "repo") + } + root = workspaceDir(p.WorkspacesRoot, p.Dir, WorkspaceRoleReviewer, agentID) + return root, filepath.Join(root, workspaceRepoName) +} + +// WriteWorkspaceOwner records the owning run for a workspace directory. It is a +// no-op for runs whose workspaces live inside the run tree, since removing the +// run already removes them. +func (p Paths) WriteWorkspaceOwner(workspace, role, agentID string) error { + if p.WorkspacesRoot == "" { + return nil + } + data, err := json.MarshalIndent(WorkspaceOwner{ + SchemaVersion: workspaceOwnerSchema, + RunDir: filepath.Clean(p.Dir), + Role: role, + AgentID: agentID, + }, "", " ") + if err != nil { + return err + } + if err := fsatomic.WriteFileAtomic(filepath.Join(workspace, WorkspaceOwnerFile), append(data, '\n'), 0o600); err != nil { + return fmt.Errorf("runartifact: write workspace owner: %w", err) + } + return nil +} + +// ReadWorkspaceOwner reads the ownership record of one workspace directory. +func ReadWorkspaceOwner(workspace string) (WorkspaceOwner, error) { + path := filepath.Join(workspace, WorkspaceOwnerFile) + data, err := os.ReadFile(path) // #nosec G304 -- workspace paths are listed from the cr-owned workspaces root. + if err != nil { + return WorkspaceOwner{}, err + } + var owner WorkspaceOwner + if err := json.Unmarshal(data, &owner); err != nil { + return WorkspaceOwner{}, fmt.Errorf("runartifact: %s: %w", path, err) + } + if owner.SchemaVersion != workspaceOwnerSchema || strings.TrimSpace(owner.RunDir) == "" { + return WorkspaceOwner{}, fmt.Errorf("runartifact: %s: invalid workspace owner", path) + } + return owner, nil +} + +// workspacesRootForRunDir returns /ws for a run directory in the standard +// /runs/.../run- layout, and "" for any other directory, such as a +// caller-owned benchmark or test directory. +func workspacesRootForRunDir(dir string) string { + if strings.TrimSpace(dir) == "" { + return "" + } + clean := filepath.Clean(dir) + if !strings.HasPrefix(filepath.Base(clean), "run-") { + return "" + } + runs := clean + for range runDirDepth { + runs = filepath.Dir(runs) + } + if filepath.Base(runs) != "runs" { + return "" + } + return filepath.Join(filepath.Dir(runs), statepaths.WorkspacesDirName) +} + +func workspaceDir(root, runDir, role, agentID string) string { + hash := sha256.New() + for _, part := range []string{filepath.Clean(runDir), role, agentID} { + var size [8]byte + binary.BigEndian.PutUint64(size[:], uint64(len(part))) + hash.Write(size[:]) + hash.Write([]byte(part)) + } + return filepath.Join(root, hex.EncodeToString(hash.Sum(nil))[:workspaceKeyLength]) +} diff --git a/internal/runartifact/workspaces_test.go b/internal/runartifact/workspaces_test.go new file mode 100644 index 00000000..d408d950 --- /dev/null +++ b/internal/runartifact/workspaces_test.go @@ -0,0 +1,148 @@ +package runartifact + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/agents" + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + "github.com/open-cli-collective/codereview-cli/internal/statepaths" +) + +// windowsGitDirLimit mirrors Git for Windows: PATH_MAX 260 minus the 40 +// characters setup.c reserves when checking an explicit GIT_DIR. +const windowsGitDirLimit = 220 + +func workspaceTestRun(t *testing.T, dataRoot, owner, repo string, number int, runID string) Paths { + t.Helper() + ref := gitprovider.PRRef{Host: "github.com", Owner: owner, Repo: repo, Number: number} + pr := gitprovider.PR{ + Ref: ref, + Head: gitprovider.PRBranchRef{SHA: "0123456789abcdef0123456789abcdef01234567"}, + Base: gitprovider.PRBranchRef{SHA: "fedcba9876543210fedcba9876543210fedcba98"}, + } + paths, err := ForRun(statepaths.NewLayout(dataRoot, ""), ref, pr, "signalft-reviewer-profile", "signalft-reviewer-bot", runID) + if err != nil { + t.Fatalf("ForRun: %v", err) + } + return paths +} + +// The budget that broke Windows reviews: every checkout's .git path must fit +// Git for Windows' GIT_DIR limit for the longest names GitHub allows, the +// longest reviewer IDs, and the coverage-repair suffix, with a data root longer +// than the Windows default. +func TestWorkspaceGitDirsFitWindowsLimitForWorstCaseNames(t *testing.T) { + // Longer than C:\Users\<20-char user>\AppData\Local\cr\data. + dataRoot := filepath.FromSlash("C:/Users/" + strings.Repeat("u", 40) + "/AppData/Local/cr/data") + owner := strings.Repeat("o", 39) // GitHub's owner maximum. + repo := strings.Repeat("r", 100) // GitHub's repository maximum. + paths := workspaceTestRun(t, dataRoot, owner, repo, 999999, "123e4567-e89b-12d3-a456-426614174000") + + if paths.WorkspacesRoot != filepath.Join(dataRoot, statepaths.WorkspacesDirName) { + t.Fatalf("WorkspacesRoot = %q, want %q", paths.WorkspacesRoot, filepath.Join(dataRoot, statepaths.WorkspacesDirName)) + } + longAgent := "architecture:" + strings.Repeat("a", 120) + repos := map[string]string{"workbench": paths.WorkbenchRepoDir} + for _, agentID := range []string{"architecture:solid-reviewer-agnostic", longAgent, longAgent + agents.ReviewerCoverageRepairSuffix} { + _, repoDir := paths.ReviewerWorkspace(agentID, "unused") + repos[agentID] = repoDir + } + // The data root plus \ws\<12>\r\.git, whatever the PR or reviewer. + wantLen := len(dataRoot) + len(filepath.Join("x", statepaths.WorkspacesDirName, strings.Repeat("k", workspaceKeyLength), workspaceRepoName, ".git")) - 1 + for name, repoDir := range repos { + gitDir := filepath.Join(repoDir, ".git") + if !strings.HasPrefix(gitDir, paths.WorkspacesRoot+string(filepath.Separator)) { + t.Fatalf("%s .git path %q is outside the workspaces root %q", name, gitDir, paths.WorkspacesRoot) + } + if len(gitDir) != wantLen { + t.Fatalf("%s .git path is %d characters, want data root plus fixed suffix = %d: %q", name, len(gitDir), wantLen, gitDir) + } + if len(gitDir) > windowsGitDirLimit { + t.Fatalf("%s .git path is %d characters, over Git for Windows' %d: %q", name, len(gitDir), windowsGitDirLimit, gitDir) + } + } +} + +func TestWorkspaceDirsAreStableAndDistinct(t *testing.T) { + dataRoot := t.TempDir() + run := workspaceTestRun(t, dataRoot, "SignalFT", "Monit.PositivePay.Svc", 12, "run-a") + other := workspaceTestRun(t, dataRoot, "SignalFT", "Monit.PositivePay.Svc", 12, "run-b") + + if resumed := FromDir(run.Dir); resumed != run { + t.Fatalf("FromDir(run.Dir) = %#v, want the ForRun paths %#v so a resumed run finds its workspaces", resumed, run) + } + alpha, _ := run.ReviewerWorkspace("harness:alpha", "unused") + if again, _ := run.ReviewerWorkspace("harness:alpha", "unused"); again != alpha { + t.Fatalf("reviewer workspace changed between calls: %q then %q", alpha, again) + } + seen := map[string]string{} + for name, dir := range map[string]string{ + "workbench": run.WorkbenchWorkspace(), + "alpha": alpha, + "alpha repair": first(run.ReviewerWorkspace("harness:alpha"+agents.ReviewerCoverageRepairSuffix, "unused")), + "beta": first(run.ReviewerWorkspace("harness:beta", "unused")), + "other run workbench": other.WorkbenchWorkspace(), + "other run alpha": first(other.ReviewerWorkspace("harness:alpha", "unused")), + } { + if previous, ok := seen[dir]; ok { + t.Fatalf("%s and %s share workspace %q", previous, name, dir) + } + seen[dir] = name + } +} + +func TestNonStandardRunDirKeepsWorkspacesInWorkbench(t *testing.T) { + for _, dir := range []string{ + t.TempDir(), + filepath.Join(t.TempDir(), "run-benchmark"), + filepath.Join(t.TempDir(), "elsewhere", "a", "b", "c", "d", "run-x"), + } { + paths := FromDir(dir) + if paths.WorkspacesRoot != "" || paths.WorkbenchWorkspace() != "" { + t.Fatalf("FromDir(%q) workspaces root = %q, want none outside the data-root layout", dir, paths.WorkspacesRoot) + } + if want := filepath.Join(dir, "workbench", "repo"); paths.WorkbenchRepoDir != want { + t.Fatalf("WorkbenchRepoDir = %q, want %q", paths.WorkbenchRepoDir, want) + } + root, repo := paths.ReviewerWorkspace("go:tests", "go-tests-1234") + if want := filepath.Join(dir, "workbench", "reviewers", "go-tests-1234"); root != want || repo != filepath.Join(want, "repo") { + t.Fatalf("ReviewerWorkspace = %q, %q; want %q and its repo child", root, repo, want) + } + if err := paths.WriteWorkspaceOwner(root, WorkspaceRoleReviewer, "go:tests"); err != nil { + t.Fatalf("WriteWorkspaceOwner: %v", err) + } + if _, err := os.Stat(filepath.Join(root, WorkspaceOwnerFile)); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("owner record stat err = %v, want none written inside the run tree", err) + } + } +} + +func TestWorkspaceOwnerRoundTrip(t *testing.T) { + run := workspaceTestRun(t, t.TempDir(), "SignalFT", "signal-adminapp-frontend", 7, "run-owner") + root, _ := run.ReviewerWorkspace("harness:alpha", "unused") + if err := os.MkdirAll(root, 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := run.WriteWorkspaceOwner(root, WorkspaceRoleReviewer, "harness:alpha"); err != nil { + t.Fatalf("WriteWorkspaceOwner: %v", err) + } + owner, err := ReadWorkspaceOwner(root) + if err != nil { + t.Fatalf("ReadWorkspaceOwner: %v", err) + } + if owner.RunDir != filepath.Clean(run.Dir) || owner.Role != WorkspaceRoleReviewer || owner.AgentID != "harness:alpha" { + t.Fatalf("owner = %#v, want run %q reviewer harness:alpha", owner, run.Dir) + } + if err := os.WriteFile(filepath.Join(root, WorkspaceOwnerFile), []byte(`{"schema_version":1}`), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if _, err := ReadWorkspaceOwner(root); err == nil { + t.Fatal("ReadWorkspaceOwner accepted an owner record without a run directory") + } +} + +func first(root, _ string) string { return root } diff --git a/internal/statepaths/statepaths.go b/internal/statepaths/statepaths.go index d0476f4a..98cdc212 100644 --- a/internal/statepaths/statepaths.go +++ b/internal/statepaths/statepaths.go @@ -174,6 +174,16 @@ func (l Layout) LockFile(spec LockSpec) (string, error) { return filepath.Join(l.DataRoot, "locks", prKey+"__"+spec.HeadSHA[:7]+"__"+keyHash+".lock"), nil } +// WorkspacesDirName is the data-root child holding git workspaces. +const WorkspacesDirName = "ws" + +// WorkspacesRoot returns the flat directory holding run git workspaces. They +// live outside the deeply nested run artifact tree so a workspace's .git path +// stays short: Git for Windows refuses a GIT_DIR longer than 220 characters. +func (l Layout) WorkspacesRoot() string { + return filepath.Join(l.DataRoot, WorkspacesDirName) +} + // ActiveRunsLock returns the data-root-wide advisory lock path that // serializes destructive retention against live runs: every review run holds // it shared for its duration, and pruning requires it exclusively. diff --git a/internal/workbench/git_dir_length_test.go b/internal/workbench/git_dir_length_test.go new file mode 100644 index 00000000..0fbff826 --- /dev/null +++ b/internal/workbench/git_dir_length_test.go @@ -0,0 +1,73 @@ +package workbench + +import ( + "context" + "path/filepath" + "strings" + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/runartifact" +) + +func withGOOS(t *testing.T, value string) { + t.Helper() + previous := goos + goos = value + t.Cleanup(func() { goos = previous }) +} + +func TestCheckGitDirLengthEnforcesWindowsLimitOnly(t *testing.T) { + // A repo dir whose .git path lands exactly on the limit, and one past it. + atLimit := filepath.Join(string(filepath.Separator)+strings.Repeat("d", windowsGitDirLimit-len(filepath.Join("x", ".git"))), "") + if got := len(filepath.Join(atLimit, ".git")); got != windowsGitDirLimit { + t.Fatalf("fixture .git length = %d, want %d", got, windowsGitDirLimit) + } + overLimit := atLimit + "d" + + withGOOS(t, "windows") + if err := checkGitDirLength(atLimit); err != nil { + t.Fatalf("checkGitDirLength at the limit: %v", err) + } + err := checkGitDirLength(overLimit) + if err == nil { + t.Fatal("checkGitDirLength accepted a .git path over the Windows limit") + } + for _, want := range []string{"221 characters", "at most 220", `%LOCALAPPDATA%\cr\data`} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("error %q does not mention %q", err, want) + } + } + + for _, platform := range []string{"linux", "darwin"} { + withGOOS(t, platform) + if err := checkGitDirLength(overLimit + strings.Repeat("d", 1000)); err != nil { + t.Fatalf("checkGitDirLength on %s: %v, want no limit", platform, err) + } + } +} + +// A reviewer workspace too long for Git for Windows fails before any git +// command runs, with the fix in the error, instead of mid-clone with +// "'$GIT_DIR' too big". +func TestReviewerWorkspaceRejectsOverlongGitDirBeforeCloning(t *testing.T) { + withGOOS(t, "windows") + dataRoot := filepath.Join(string(filepath.Separator)+strings.Repeat("d", 200), "cr", "data") + runDir := filepath.Join(dataRoot, "runs", "github.com_o_r_1", "h", "b", "s", "run-x") + artifacts := runartifact.FromDir(runDir) + if artifacts.WorkspacesRoot == "" { + t.Fatal("fixture run dir is not in the data-root layout") + } + var calls [][]string + deps := Deps{GitCommand: func(_ context.Context, _ string, args ...string) ([]byte, error) { + calls = append(calls, args) + return nil, nil + }} + + _, _, err := prepareReviewerWorkspace(context.Background(), deps, artifacts, "abc", "harness:alpha", nil, defaultReviewerWorkspaceToolOutputBytes) + if err == nil || !strings.Contains(err.Error(), "Git for Windows accepts at most 220") { + t.Fatalf("prepareReviewerWorkspace error = %v, want the Windows GIT_DIR limit", err) + } + if len(calls) != 0 { + t.Fatalf("git calls = %q, want none before the length check", calls) + } +} diff --git a/internal/workbench/workbench.go b/internal/workbench/workbench.go index 39c47df3..5d00c7a0 100644 --- a/internal/workbench/workbench.go +++ b/internal/workbench/workbench.go @@ -12,6 +12,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "slices" "sort" "strings" @@ -30,8 +31,16 @@ const ( // workbenchHeadRef gives the workbench a ref so it is a clonable repository. workbenchHeadRef = "refs/heads/cr-review-head" defaultReviewerWorkspaceToolOutputBytes = 32 * 1024 + // windowsGitDirLimit is the longest GIT_DIR Git for Windows accepts: its + // PATH_MAX is 260, and setup.c rejects an explicit GIT_DIR longer than + // PATH_MAX - 40. Clones and fetches hand GIT_DIR to their index-pack child, + // so a longer checkout path fails mid-transfer with "'$GIT_DIR' too big". + windowsGitDirLimit = 220 ) +// goos is the platform the Git path limit is checked for; tests override it. +var goos = runtime.GOOS + // ErrUnsafeFetchRef marks a provider ref that cannot be passed to Git safely. var ErrUnsafeFetchRef = errors.New("workbench: unsafe fetch ref") @@ -135,14 +144,31 @@ func (p *RunPreparer) prepare(ctx context.Context, req Request) error { return nil } - if err := os.RemoveAll(req.Artifacts.WorkbenchDir); err != nil { - return fmt.Errorf("pipeline: reset workbench dir: %w", err) + if err := checkGitDirLength(req.Artifacts.WorkbenchRepoDir); err != nil { + return err + } + workspace := req.Artifacts.WorkbenchWorkspace() + for _, dir := range []string{req.Artifacts.WorkbenchDir, workspace} { + if dir == "" { + continue + } + if err := os.RemoveAll(dir); err != nil { + return fmt.Errorf("pipeline: reset workbench dir: %w", err) + } } - for _, dir := range []string{req.Artifacts.WorkbenchDir, req.Artifacts.WorkbenchScratch} { + for _, dir := range []string{req.Artifacts.WorkbenchDir, req.Artifacts.WorkbenchScratch, workspace} { + if dir == "" { + continue + } if err := os.MkdirAll(dir, 0o700); err != nil { return fmt.Errorf("pipeline: create workbench dir: %w", err) } } + if workspace != "" { + if err := req.Artifacts.WriteWorkspaceOwner(workspace, runartifact.WorkspaceRoleWorkbench, ""); err != nil { + return err + } + } baseRemoteURL, err := branchRemoteURL(req.ReviewPR.Base) if err != nil { @@ -151,6 +177,9 @@ func (p *RunPreparer) prepare(ctx context.Context, req Request) error { if _, err := p.deps.gitCommand(ctx, "", "init", req.Artifacts.WorkbenchRepoDir); err != nil { return fmt.Errorf("pipeline: initialize workbench repo: %w", err) } + if _, err := p.deps.gitCommand(ctx, req.Artifacts.WorkbenchRepoDir, "config", "core.longpaths", "true"); err != nil { + return fmt.Errorf("pipeline: configure workbench long paths: %w", err) + } if _, err := p.deps.gitCommand(ctx, req.Artifacts.WorkbenchRepoDir, "remote", "add", "origin", baseRemoteURL); err != nil { return fmt.Errorf("pipeline: configure workbench origin: %w", err) } @@ -487,8 +516,10 @@ func prepareReviewerWorkspace(ctx context.Context, deps Deps, artifacts runartif return llm.ReviewerWorkspaceRequest{}, nil, fmt.Errorf("pipeline: agent ID is required for reviewer workspace") } segment := ReviewerWorkspaceSegment(agentID) - workspaceRoot := filepath.Join(artifacts.WorkbenchDir, "reviewers", segment) - workspaceRepo := filepath.Join(workspaceRoot, "repo") + workspaceRoot, workspaceRepo := artifacts.ReviewerWorkspace(agentID, segment) + if err := checkGitDirLength(workspaceRepo); err != nil { + return llm.ReviewerWorkspaceRequest{}, nil, err + } workspaceScratch := filepath.Join(artifacts.WorkbenchScratch, segment) for _, dir := range []string{workspaceRoot, workspaceScratch} { if err := os.RemoveAll(dir); err != nil { @@ -509,9 +540,15 @@ func prepareReviewerWorkspace(ctx context.Context, deps Deps, artifacts runartif } return cleanupErr } + if err := artifacts.WriteWorkspaceOwner(workspaceRoot, runartifact.WorkspaceRoleReviewer, agentID); err != nil { + _ = cleanup() + return llm.ReviewerWorkspaceRequest{}, nil, err + } // Use Git transport instead of copying the source object directory. A // background repack can remove a pack while local-copy cloning reads it. - if _, err := deps.gitCommand(ctx, "", "clone", "--no-local", artifacts.WorkbenchRepoDir, workspaceRepo); err != nil { + // core.longpaths lets Git for Windows check out files whose full path + // exceeds 260 characters; other platforms ignore it. + if _, err := deps.gitCommand(ctx, "", "clone", "--no-local", "--config", "core.longpaths=true", artifacts.WorkbenchRepoDir, workspaceRepo); err != nil { _ = cleanup() return llm.ReviewerWorkspaceRequest{}, nil, fmt.Errorf("pipeline: clone reviewer workspace: %w", err) } @@ -562,6 +599,19 @@ func prepareReviewerWorkspace(ctx context.Context, deps Deps, artifacts runartif return workspace, cleanup, nil } +// checkGitDirLength fails early, with the fix, when a checkout's .git path +// would exceed what Git for Windows accepts. +func checkGitDirLength(repoDir string) error { + if goos != "windows" { + return nil + } + gitDir := filepath.Join(repoDir, ".git") + if n := len(gitDir); n > windowsGitDirLimit { + return fmt.Errorf("pipeline: git directory %s is %d characters, and Git for Windows accepts at most %d; move the cr data directory (%%LOCALAPPDATA%%\\cr\\data) to a shorter path", gitDir, n, windowsGitDirLimit) + } + return nil +} + func isReviewerWorkspaceEscapePath(clean string) bool { return clean == "." || clean == "" || filepath.IsAbs(clean) || clean == ".." || strings.HasPrefix(clean, ".."+string(os.PathSeparator)) } From 03689af739a0c4eaae3a401b7324dcdb40a95392 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Mon, 9 Nov 2026 15:49:36 -0500 Subject: [PATCH 2/3] fix(datalifecycle): keep workspaces whose owner record cannot be read Only a missing owner record marks a workspace as unowned. A record that exists but cannot be read or parsed keeps the workspace and adds a prune warning, so a permission or I/O error cannot delete a live run's checkout. --- internal/datalifecycle/datalifecycle.go | 39 ++++++++++++-------- internal/datalifecycle/workspaces_test.go | 44 +++++++++++++++++++++++ 2 files changed, 68 insertions(+), 15 deletions(-) diff --git a/internal/datalifecycle/datalifecycle.go b/internal/datalifecycle/datalifecycle.go index e954285a..2f8303ad 100644 --- a/internal/datalifecycle/datalifecycle.go +++ b/internal/datalifecycle/datalifecycle.go @@ -136,7 +136,7 @@ func Show(ctx context.Context, opts Options) (Stats, error) { if err != nil { return Stats{}, err } - workspaceOrphans, err := orphanWorkspaces(opts.Layout, opts.now().Add(-OrphanGrace), nil) + workspaceOrphans, _, err := orphanWorkspaces(opts.Layout, opts.now().Add(-OrphanGrace), nil) if err != nil { return Stats{}, err } @@ -251,11 +251,12 @@ func Prune(ctx context.Context, opts Options, prune PruneOptions) (PruneResult, for _, orphan := range orphanItems { removing[filepath.Clean(orphan.Path)] = true } - workspaceOrphans, err := orphanWorkspaces(opts.Layout, opts.now().Add(-OrphanGrace), removing) + workspaceOrphans, warnings, err := orphanWorkspaces(opts.Layout, opts.now().Add(-OrphanGrace), removing) if err != nil { orphanSpan.End(err) return result, err } + result.Warnings = append(result.Warnings, warnings...) orphanSpan.End(nil) result.OrphansRemoved = append(orphanItems, workspaceOrphans...) return result, nil @@ -301,11 +302,12 @@ func Prune(ctx context.Context, opts Options, prune PruneOptions) (PruneResult, } // Run directories are gone by now, so this also catches the workspaces of // every run deleted above. - workspaceOrphans, err := orphanWorkspaces(opts.Layout, opts.now().Add(-OrphanGrace), nil) + workspaceOrphans, warnings, err := orphanWorkspaces(opts.Layout, opts.now().Add(-OrphanGrace), nil) if err != nil { orphanSpan.End(err) return result, err } + result.Warnings = append(result.Warnings, warnings...) for _, orphan := range workspaceOrphans { if err := opts.removeAll()(orphan.Path); err != nil && !errors.Is(err, fs.ErrNotExist) { result.Warnings = append(result.Warnings, fmt.Sprintf("failed to remove orphan workspace at %s: %v", orphan.Path, err)) @@ -501,23 +503,26 @@ func orphanItems(layout statepaths.Layout, runs []ledger.Run, graceCutoff time.T } // orphanWorkspaces lists git workspaces whose owning run directory no longer -// exists or is in removing. A workspace without a readable owner record (a run -// that stopped between creating the directory and recording its owner) is -// swept once it is older than graceCutoff. Symlinked entries are never -// followed or removed. -func orphanWorkspaces(layout statepaths.Layout, graceCutoff time.Time, removing map[string]bool) ([]OrphanItem, error) { +// exists or is in removing. A workspace with no owner record (a run that +// stopped between creating the directory and recording its owner) is swept +// once it is older than graceCutoff. An owner record that exists but cannot be +// read or parsed proves nothing about ownership, so that workspace is kept and +// reported in the returned warnings. Symlinked entries are never followed or +// removed. +func orphanWorkspaces(layout statepaths.Layout, graceCutoff time.Time, removing map[string]bool) ([]OrphanItem, []string, error) { root := layout.WorkspacesRoot() if err := rejectSymlink(root); err != nil { - return nil, err + return nil, nil, err } entries, err := os.ReadDir(root) if errors.Is(err, fs.ErrNotExist) { - return nil, nil + return nil, nil, nil } if err != nil { - return nil, err + return nil, nil, err } var items []OrphanItem + var warnings []string for _, entry := range entries { if !entry.IsDir() || entry.Type()&fs.ModeSymlink != 0 { continue @@ -525,30 +530,34 @@ func orphanWorkspaces(layout statepaths.Layout, graceCutoff time.Time, removing path := filepath.Join(root, entry.Name()) orphaned, err := workspaceOrphaned(path, entry, graceCutoff, removing) if err != nil { - return nil, err + warnings = append(warnings, fmt.Sprintf("kept workspace %s: cannot determine its owner: %v", path, err)) + continue } if !orphaned { continue } bytes, err := dirBytes(path) if err != nil { - return nil, err + return nil, nil, err } items = append(items, OrphanItem{Path: path, Bytes: bytes}) } sort.Slice(items, func(i, j int) bool { return items[i].Path < items[j].Path }) - return items, nil + return items, warnings, nil } func workspaceOrphaned(path string, entry fs.DirEntry, graceCutoff time.Time, removing map[string]bool) (bool, error) { owner, err := runartifact.ReadWorkspaceOwner(path) - if err != nil { + if errors.Is(err, fs.ErrNotExist) { info, infoErr := entry.Info() if infoErr != nil { return false, infoErr } return !info.ModTime().After(graceCutoff), nil } + if err != nil { + return false, err + } runDir := filepath.Clean(owner.RunDir) if removing[runDir] { return true, nil diff --git a/internal/datalifecycle/workspaces_test.go b/internal/datalifecycle/workspaces_test.go index cd04a077..a57ae1b6 100644 --- a/internal/datalifecycle/workspaces_test.go +++ b/internal/datalifecycle/workspaces_test.go @@ -6,6 +6,8 @@ import ( "os" "path/filepath" "reflect" + "runtime" + "strings" "testing" "github.com/open-cli-collective/codereview-cli/internal/ledger" @@ -130,3 +132,45 @@ func TestShowCountsWorkspaceBytesAndOrphans(t *testing.T) { t.Fatalf("ArtifactBytes = %d, want run artifacts plus workspaces = %d", stats.ArtifactBytes, want) } } + +// An owner record that exists but cannot be read or parsed says nothing about +// whether the run is gone, so prune keeps the workspace and warns instead of +// treating it as unowned. +func TestPruneKeepsWorkspacesWhoseOwnerCannotBeRead(t *testing.T) { + layout := testLayout(t) + root := layout.WorkspacesRoot() + corrupt := writeWorkspace(t, root, "aaaaaaaaaaaa", "") + writeFile(t, filepath.Join(corrupt, runartifact.WorkspaceOwnerFile), "{not json") + backdate(t, corrupt, testNow().Add(-2*OrphanGrace)) + kept := []string{corrupt} + + if runtime.GOOS != "windows" && os.Geteuid() != 0 { + unreadable := writeWorkspace(t, root, "bbbbbbbbbbbb", filepath.Join(layout.DataRoot, "runs", "gone")) + ownerFile := filepath.Join(unreadable, runartifact.WorkspaceOwnerFile) + if err := os.Chmod(ownerFile, 0o000); err != nil { + t.Fatalf("Chmod: %v", err) + } + t.Cleanup(func() { _ = os.Chmod(ownerFile, 0o600) }) + backdate(t, unreadable, testNow().Add(-2*OrphanGrace)) + kept = append(kept, unreadable) + } + + result, err := Prune(context.Background(), Options{Layout: layout, Store: &fakeStore{}, Now: testNow}, PruneOptions{}) + if err != nil { + t.Fatalf("Prune: %v", err) + } + if len(result.OrphansRemoved) != 0 { + t.Fatalf("orphans removed = %#v, want workspaces with unreadable owners kept", orphanPaths(result.OrphansRemoved)) + } + if len(result.Warnings) != len(kept) { + t.Fatalf("warnings = %#v, want one per kept workspace %#v", result.Warnings, kept) + } + for i, path := range kept { + if _, err := os.Stat(path); err != nil { + t.Fatalf("%s removed: %v", path, err) + } + if !strings.Contains(result.Warnings[i], path) { + t.Fatalf("warning %q does not name %s", result.Warnings[i], path) + } + } +} From 19801b8f1387bb244898ffe8494d077e22aba6c3 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Mon, 9 Nov 2026 15:51:02 -0500 Subject: [PATCH 3/3] docs: distinguish missing and unreadable workspace owner records --- docs/checkout-native-review-contract.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/checkout-native-review-contract.md b/docs/checkout-native-review-contract.md index c27986bb..5015a7ac 100644 --- a/docs/checkout-native-review-contract.md +++ b/docs/checkout-native-review-contract.md @@ -95,7 +95,10 @@ characters; other platforms ignore the setting. `cr data prune` removes a workspace whose `owner.json` names a run directory that no longer exists (including runs that prune itself deletes), and one with -no readable owner record once it is older than the orphan grace period. +no `owner.json` at all once it is older than the orphan grace period. An +`owner.json` that exists but cannot be read or parsed proves nothing about +ownership: prune keeps that workspace and reports a warning rather than risk +deleting a live run's checkout. `cr data purge` removes the whole data root, workspaces included. Run directories outside the standard `/runs/.../run-` layout, such as caller-owned benchmark directories, keep their checkouts under `workbench/` as