Repository navigation
fix(host-service): classify simple-git's missing-directory construct error as a typed NOT_FOUND - #6495
Conversation
…error simple-git's factory re-checks baseDir and throws before spawning git, so a worktree deleted between resolveWorktreePath's existsSync guard and the git call escaped git.getStatus as an unhandled 500. Classify it as the same NOT_FOUND / WORKTREE_MISSING the guard already produces. Refs HOST-SERVICE-W
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe Git error classifier now maps simple-git missing-directory errors to ChangesWorktree error classification
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change classifies one known missing-directory condition as NOT_FOUND while preserving other construction failures as 500s; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Problem
git.getStatusreports ~23 unhandled 500s/day (174 lifetime, 159 in 7d, still arriving on 1.21.0) forCannot use simple-git on a directory that does not exist.User-visible harm: none, and I want to be straight about that. Nothing on this path branches on the error code or cause — the v2 query client sets
retry: false, and the host-service error formatter does not forwardcauseto the wire, so the renderer renders identically before and after this change. The harm is to us, and it is twofold:sentryMiddlewaredocuments the invariant "anything still INTERNAL_SERVER_ERROR at this boundary is a bug." For the app's highest-traffic git procedure that is currently false, and realgit.getStatusbugs land under ~23 noise events/day.resolveWorktreePath(two lines earlier in the same procedure) and the classifier's existingCWD_GONEbranch both already report this exact physical condition asNOT_FOUND/WORKTREE_MISSING. Only the construct-time spelling of it escapes untyped.I considered archiving instead. It's dominated: the issue has already been archived-and-regressed once (substatus
regressed), and archiving buries the group that future genuine 500s on this procedure would land in. A pre-constructexistsSyncre-check is not an option either — it's the same TOCTOU window, just narrower. Classification is the only correct instrument for a race. No in-flight PR restructures this path (nearest neighbours #6260 and #6446 are prior extensions of this same classifier).Root cause
resolveWorktreePathchecksexistsSyncand hands the path onward.createUserSimpleGit(worktreePath)reaches simple-git'sgitInstanceFactory, which re-checksfolderExists(config.baseDir)and throwsGitConstructErrorbefore any git process is spawned. Between those two steps the directory can be removed — the user deleted the workspace, an external tool pruned it, a cleanup ran concurrently. In the reported events the worker's status run had already succeeded and returned abaseRefFetchTarget, so the deletion lands during the coordinator's client construction.Because that error is simple-git's own text and not one of the git-CLI conditions the classifier knows, it falls through
rethrowEnvironmentalGitErroras a 500.Fix
One branch in
classify-git-error.ts, mapping simple-git's construct message to the sameNOT_FOUND/WORKTREE_MISSINGthe sibling guard already emits. Message preserved verbatim, as every other branch in this file does.On the matcher — the error name is not usable here, and I verified this rather than assuming it. simple-git's
GitErrorbase class never assignsthis.name, so aGitConstructErrorarrives named"Error". Production confirms it: the Sentry issue title readsError: Cannot use simple-git..., notGitConstructError: .... Aerror.name === "GitConstructError"check would have compiled, read as the more structural choice, and been dead code that never fired.serializeWorkerErrorindependently rules out anything richer: crossing the worker boundary it keeps onlyname/message/stack/code, dropping theconfig.baseDirown-property thatGitConstructErrorotherwise carries. The message is the only signal that survives every path, and it is safe to key on — it is a hard-coded template literal in simple-git, never localised and never interpolated with user content, unlike git's own output.Keying on the sentence is also the narrower choice, which is what the type-vs-condition distinction actually asks for.
GitConstructErroris the type simple-git would also use for construction mistakes that are ours; those must keep reporting as 500s, and they do, because they carry a different sentence.Still reporting as 500s on this path — everything except the four environmental conditions now enumerated in the classifier. Concretely: any
GitConstructErrorwhose message is not the missing-directory sentence (i.e. our own misconfiguration of the client);index.lockcontention; dubious-ownership rejections; bad revisions and unknown refs;cannot chdir to work tree; worker-pool timeouts and crashes; and every other git failure. The added negative tests pin the first of those.cause: { kind: "WORKTREE_MISSING" }is reused rather than invented. To be explicit: nothing currently reads it on this path — the host-serviceerrorFormatteronly forwardsteardownFailure,projectNotSetupanddeleteInProgress, socausenever reaches the renderer here. It is included solely so this throw site matches the two sibling throw sites for the identical condition (resolve-worktree.ts:27and theCWD_GONEbranch above it); emitting a bare error would make this the only one of the three that drops the tag.Adjacent throw sites, left alone
gitStatusSnapshotTask,gitFetchBaseRefTask,gitCommitFilesTaskand the other worker tasks inworkers/tasks/git.tseach callcreateUserSimpleGit(worktreePath)and can raise the same construct error. Their callers route through this same classifier where they classify at all; the ones that don't are outside this root cause and are not touched here.Verification
bunx biome checkon both changed files:cd packages/host-service && bun run typecheck:Negative tests were written first and run before the change — the two over-match guards and the misconfiguration guard passed, the positive case failed, confirming the branch was absent rather than accidentally already covered:
After the change,
bun test packages/host-service/src/trpc/router/git:One of the added tests asserts the name observation directly against the real dependency, so a future simple-git release that starts setting
this.namefails loudly here instead of silently changing what this branch matches.Repo-wide
bun run lintwas not run — it hangs on cross-worktree bun locks.Refs HOST-SERVICE-W
https://claude.ai/code/session_1d4c6d97-bdd8-4c49-b4b6-9182b085df16
Summary by cubic
Classifies
simple-git’s “Cannot use simple-git on a directory that does not exist” construct error as NOT_FOUND/WORKTREE_MISSING in the host-service git classifier. Previously,git.getStatusreturned INTERNAL_SERVER_ERROR for this race when the worktree was deleted between checks; user-visible behavior is unchanged, but Sentry noise drops and classification now matches sibling guards.packages/host-service/src/trpc/router/git/utils/classify-git-error.tskeyed to the exactsimple-gitsentence; throwsNOT_FOUNDwithcause: { kind: "WORKTREE_MISSING" }. Other construct errors and all non-environmental failures remain 500s.GitConstructErrorarrives named “Error”, so name-based matching would be dead code.Written for commit 7cd216e. Summary will update on new commits.
Summary by CodeRabbit