Repository navigation
fix(workbench): keep git checkouts within Git for Windows' GIT_DIR limit - #717
Merged
Merged
Conversation
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 <data>/ws/<key>/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.
![rianjs-bot[bot]](https://avatars.githubusercontent.com/u/7378790?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 385cf404f96c
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 0 |
| structure:repo-health | 1 |
structure:repo-health (1 finding)
Major - internal/datalifecycle/datalifecycle.go:545
A failure to read an owner record does not establish that the workspace has no owner. For example, a permission or transient I/O error on an existing owner file causes an older workspace to be classified as orphaned and deleted by automatic retention even while its run directory still exists. Treat only a missing owner file as an unowned workspace; propagate or warn and skip other read errors, and add a test proving an unreadable owner cannot trigger deletion.
Reviewer Coverage
Complete coverage details are retained in the local coverage.json artifact.
go:implementation-tests— complete (broad); scope: 12 files; examples:internal/datalifecycle/datalifecycle.go,internal/datalifecycle/workspaces_test.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/workbench_cleanup_test.go; 7 omitted; body-inspected: 12 files; examples:internal/datalifecycle/datalifecycle.go,internal/datalifecycle/workspaces_test.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/workbench_cleanup_test.go; 7 omitted; skipped: none; constraints: Review was limited to the assigned Go implementation and test scope; tests were not executed.policies:conventions— complete (broad); scope: 8 files; examples:README.md,docs/checkout-native-review-contract.md,docs/review-guidance.md,internal/datalifecycle/datalifecycle.go,internal/runartifact/runartifact.go; 3 omitted; body-inspected: 8 files; examples:README.md,docs/checkout-native-review-contract.md,docs/review-guidance.md,internal/datalifecycle/datalifecycle.go,internal/runartifact/runartifact.go; 3 omitted; skipped: none; constraints: Review focused on convention drift in the assigned changes; shared standards were not provided in the review context.structure:repo-health— complete (broad); scope: 9 files; examples:internal/datalifecycle/datalifecycle.go,internal/datalifecycle/workspaces_test.go,internal/pipeline/pipeline.go,internal/pipeline/workbench_cleanup_test.go,internal/runartifact/runartifact.go; 4 omitted; body-inspected: 9 files; examples:internal/datalifecycle/datalifecycle.go,internal/datalifecycle/workspaces_test.go,internal/pipeline/pipeline.go,internal/pipeline/workbench_cleanup_test.go,internal/runartifact/runartifact.go; 4 omitted; skipped: none; constraints: Review was limited to the assigned files and their lifecycle interactions; tests were not run.
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 1m 00s | gpt-6.1-terra | cr 0.10.367
| Field | Value |
|---|---|
| Model | gpt-6.1-terra |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | codex_cli · gpt-6.1-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 1m 00s wall · 1m 34s compute |
| Cost | unavailable |
| Tokens | 474.3k in / 3.0k out |
Per-workstream usage
orchestrator-selection— gpt-6.1-terra- In: 19.5k
- Out: 409
- Cache read: 12.2k
- Cache create: unavailable
- Cost: unavailable
- Duration: 11s
go:implementation-tests— gpt-6.1-terra- In: 137.2k
- Out: 566
- Cache read: 106.4k
- Cache create: unavailable
- Cost: unavailable
- Duration: 17s
policies:conventions— gpt-6.1-terra- In: 103.6k
- Out: 522
- Cache read: 83.8k
- Cache create: unavailable
- Cost: unavailable
- Duration: 26s
structure:repo-health— gpt-6.1-terra- In: 173.3k
- Out: 1.1k
- Cache read: 142.3k
- Cache create: unavailable
- Cost: unavailable
- Duration: 31s
orchestrator-rollup— gpt-6.1-terra- In: 40.8k
- Out: 482
- Cache read: 31.9k
- Cache create: unavailable
- Cost: unavailable
- Duration: 7s
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.
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 03689af739a0
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 1 |
| structure:repo-health | 0 |
policies:conventions (1 finding)
Minor - docs/checkout-native-review-contract.md:99
This says prune removes workspaces with an unreadable owner record after the grace period, but the implementation now preserves unreadable or corrupt records and reports a warning. Update the contract to distinguish missing records from unreadable or invalid ones so future maintainers do not restore the destructive behavior.
Reviewer Coverage
Complete coverage details are retained in the local coverage.json artifact.
go:implementation-tests— complete (constrained); scope: 12 files; examples:internal/datalifecycle/datalifecycle.go,internal/datalifecycle/workspaces_test.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/workbench_cleanup_test.go; 7 omitted; body-inspected: 12 files; examples:internal/datalifecycle/datalifecycle.go,internal/datalifecycle/workspaces_test.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/workbench_cleanup_test.go; 7 omitted; skipped: none; constraints: Tests were not executed during this review.policies:conventions— complete (constrained); scope: 8 files; examples:README.md,docs/checkout-native-review-contract.md,docs/review-guidance.md,internal/datalifecycle/datalifecycle.go,internal/runartifact/runartifact.go; 3 omitted; body-inspected: 8 files; examples:README.md,docs/checkout-native-review-contract.md,docs/review-guidance.md,internal/datalifecycle/datalifecycle.go,internal/runartifact/runartifact.go; 3 omitted; skipped: none; constraints: Review focused on convention drift in the assigned changes; shared standards were not provided in the review context.structure:repo-health— complete (constrained); scope: 9 files; examples:internal/datalifecycle/datalifecycle.go,internal/datalifecycle/workspaces_test.go,internal/pipeline/pipeline.go,internal/pipeline/workbench_cleanup_test.go,internal/runartifact/runartifact.go; 4 omitted; body-inspected: 9 files; examples:internal/datalifecycle/datalifecycle.go,internal/datalifecycle/workspaces_test.go,internal/pipeline/pipeline.go,internal/pipeline/workbench_cleanup_test.go,internal/runartifact/runartifact.go; 4 omitted; skipped: none; constraints: Review was limited to the assigned files and their lifecycle interactions; tests were not run.
1 PR discussion threads considered. 1 summarized; 1 resolved.
Completed in 43s | gpt-6.1-terra | cr 0.10.367
| Field | Value |
|---|---|
| Model | gpt-6.1-terra |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | codex_cli · gpt-6.1-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 43s wall · 38s compute |
| Cost | unavailable |
| Tokens | 839.5k in / 3.8k out |
Per-workstream usage
go:implementation-tests— gpt-6.1-terra- In: 235.2k
- Out: 821
- Cache read: 197.0k
- Cache create: unavailable
- Cost: unavailable
- Duration: 9s
policies:conventions— gpt-6.1-terra- In: 183.9k
- Out: 863
- Cache read: 151.8k
- Cache create: unavailable
- Cost: unavailable
- Duration: 11s
structure:repo-health— gpt-6.1-terra- In: 332.7k
- Out: 1.4k
- Cache read: 288.1k
- Cache create: unavailable
- Cost: unavailable
- Duration: 12s
orchestrator-rollup— gpt-6.1-terra- In: 87.7k
- Out: 659
- Cache read: 75.6k
- Cache create: unavailable
- Cost: unavailable
- Duration: 4s
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 19801b8f1387
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
Reviewer Coverage
Complete coverage details are retained in the local coverage.json artifact.
go:implementation-tests— complete (constrained); scope: 12 files; examples:internal/datalifecycle/datalifecycle.go,internal/datalifecycle/workspaces_test.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/workbench_cleanup_test.go; 7 omitted; body-inspected: 12 files; examples:internal/datalifecycle/datalifecycle.go,internal/datalifecycle/workspaces_test.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/workbench_cleanup_test.go; 7 omitted; skipped: none; constraints: The assigned Go files were unchanged since the prior review. Tests were not executed.policies:conventions— complete (constrained); scope: 8 files; examples:README.md,docs/checkout-native-review-contract.md,docs/review-guidance.md,internal/datalifecycle/datalifecycle.go,internal/runartifact/runartifact.go; 3 omitted; body-inspected: 8 files; examples:README.md,docs/checkout-native-review-contract.md,docs/review-guidance.md,internal/datalifecycle/datalifecycle.go,internal/runartifact/runartifact.go; 3 omitted; skipped: none; constraints: Review focused on convention drift in the assigned changes; shared standards were not provided in the review context.structure:repo-health— complete (constrained); scope: 9 files; examples:internal/datalifecycle/datalifecycle.go,internal/datalifecycle/workspaces_test.go,internal/pipeline/pipeline.go,internal/pipeline/workbench_cleanup_test.go,internal/runartifact/runartifact.go; 4 omitted; body-inspected: 9 files; examples:internal/datalifecycle/datalifecycle.go,internal/datalifecycle/workspaces_test.go,internal/pipeline/pipeline.go,internal/pipeline/workbench_cleanup_test.go,internal/runartifact/runartifact.go; 4 omitted; skipped: none; constraints: The assigned files are unchanged since the previous review. Tests were not run.
1 PR discussion threads considered. 1 summarized; 1 resolved.
Completed in 46s | gpt-6.1-terra | cr 0.10.367
| Field | Value |
|---|---|
| Model | gpt-6.1-terra |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | codex_cli · gpt-6.1-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 46s wall · 33s compute |
| Cost | unavailable |
| Tokens | 1.2M in / 4.7k out |
Per-workstream usage
go:implementation-tests— gpt-6.1-terra- In: 346.0k
- Out: 1.1k
- Cache read: 302.3k
- Cache create: unavailable
- Cost: unavailable
- Duration: 9s
policies:conventions— gpt-6.1-terra- In: 289.3k
- Out: 1.1k
- Cache read: 244.6k
- Cache create: unavailable
- Cost: unavailable
- Duration: 9s
structure:repo-health— gpt-6.1-terra- In: 456.1k
- Out: 1.7k
- Cache read: 406.4k
- Cache create: unavailable
- Cost: unavailable
- Duration: 10s
orchestrator-rollup— gpt-6.1-terra- In: 144.1k
- Out: 806
- Cache read: 126.1k
- Cache create: unavailable
- Cost: unavailable
- Duration: 4s
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On Windows,
cr reviewfails before any reviewer runs:Git checkouts now live in a flat
<data>/ws/<key>/rbesideruns/, not inside the run directory. Every.gitpath is the data root plus 23 characters, whatever the repository, PR, reviewer, or coverage-repair pass.Root cause
Git for Windows rejects a
GIT_DIRlonger thanPATH_MAX - 40.PATH_MAXis 260 there, so the limit is 220 characters.git clone --no-local(#649) passesGIT_DIRto itsindex-packchild, so the destination's.gitpath has to fit. The old local-copy clone never started that child, which is why 0.10.322 worked.Reviewer clones sat at
<data>\runs\<prKey>\<head12>\<base12>\<scope12>\run-<uuid>\workbench\reviewers\<agent>\repo\.git. The fixed segments alone take about 115 characters. That left about 105 for the data root, the PR key (host, owner, repo, number) and the agent ID together. With a data root as short asC:\crd\cr\data, the clones still came to 221–225 characters forMonit.PositivePay.Svc,Monit.InsightsEngine.Svc,Monit.NextBestAction.Svcandsignal-adminapp-frontend. A coverage-repair pass adds 16 more. When its clone fails, the review withholds approval with zero findings.#576 shortened the run path to fit under 260, but the checkout stayed nested in it and the 220 limit for
GIT_DIRwas never accounted for. Shortening segment names (hashing the agent folder, for example) would only buy back some of the room. The limit would still depend on repo name length and on any directory level added later.What changed
<data>/ws/<key>/r.<key>is a 12-character hash of the run directory, role and agent ID: stable when a run resumes, distinct between runs and reviewers. Anowner.jsonbeside each checkout records its run, role and agent. The new code is ininternal/runartifact/workspaces.go.workbench/.cr data prune(and--dry-run) removes checkouts whose run directory is gone, including runs that prune deletes in the same pass.OrphanGrace.cr data showcounts checkout bytes.cr data purgealready removes the whole data root, checkouts included.core.longpaths=trueis set on the workbench repo and reviewer clones, so Git for Windows can also check out files whose full path passes 260 characters. Other platforms ignore it..gitlength. If it's too long, setup fails with the limit and the fix (move%LOCALAPPDATA%\cr\datato a shorter path). Without the check, the clone fails partway through.<data>/runs/.../run-<id>layout, such as caller-owned benchmark directories, keep their checkouts underworkbench/as before.docs/checkout-native-review-contract.md,docs/review-guidance.md, README.Scratch directories stay under
workbench/scratch/. They aren't git directories, so this limit doesn't apply.Existing workbenches won't be reused once after upgrading: their recorded repo path no longer matches, so they are rebuilt with one extra fetch. Old in-tree checkouts are removed when that run's workbench is reset or pruned.
Validation
TestWorkspaceGitDirsFitWindowsLimitForWorstCaseNamesruns on every OS. It uses a data root longer than the Windows default, GitHub's maximum owner (39) and repository (100) lengths, a 133-character agent ID and the coverage-repair suffix. Every.gitpath equals the data root plus the fixed suffix and stays at or under 220.FromDirreproducingForRunon resume, the legacy fallback, owner records, the Windows check (it rejects before any git call and passes at exactly 220), the prune sweep (dry run and real), symlink safety, anddata showbyte counts.workbench/reviewers/...now assert the workspaces root. Coverage-repair failure injection now finds the repair clone by its owner record, since the agent name is no longer in the path. The reviewer cleanup assertion checks the real checkout location; at the old path it would have passed trivially.make checkpasses locally (tidy, fmt, lint, test, build).GOOS=windows go vetandgo buildpass for the touched packages.Type of Change
Checklist
make tidypassesmake lintpassesmake testpassesmake buildpassesRelated Issues
Regression from #649; earlier partial fix #576.