Fix worktree removal timing out on large install trees#3902
Fix worktree removal timing out on large install trees#3902jakeleventhal wants to merge 5 commits into
Effect Service Conventions: No issues found
All clear
Details
Note
Your check run agent prompt is: .macroscope/check-run-agents/effect-service-conventions.md
More information about how Check Run Agents work can be found in our Help Center.
Reviewed the changed files in scope (apps/server/src/vcs/GitVcsDriverCore.ts and apps/server/src/vcs/GitVcsDriverCore.test.ts) against the Effect service conventions.
Findings: none.
Notes on the changes:
- Imports use namespace subpath form (
import * as Schedule from "effect/Schedule",effect/Deferred,effect/Fiber,effect/testing/TestClock), consistent with existing conventions.GitCommandErrorremains a named import from@t3tools/contracts, which is the correct treatment for a whole-package import. - The new
parsePorcelainWorktreePathsis a pure helper; no service definition, tag,make, orlayerchanges were introduced. - Error construction reuses the existing
gitCommandContext, which exposes only sanitized fields (operation,command,cwd,argumentCount) and never raw args/output. The addedGitCommandErrorsites use staticdetailstrings and preserve the underlying failure ascausewhere one exists (retry path) while omittingcausefor the timeout case, matching the established pattern in this module. - The retry
whilepredicate inspects structural platform error reason tags, which is an acceptable structural check. - The test seam using
GitVcsDriver.makewithEffect.provideService(FileSystem.FileSystem, flakyFileSystem)and theLayer.provide->Layer.provideMergechange are legitimate test wiring, not fake dependency injection. The added tests are focused coverage for the new retry/pre-delete behavior, which is appropriate since backend behavior changed.
🔍 Agent activity — reasoning, no tool calls
💭 I'm examining the diff to check for convention violations in the Effect service patterns. The changes are in
GitVcsDriverCore.tsunderapps/server/src/vcs/, and I'm verifying the imports follow the correct namespace conventions — like the Schedule import from the Effect library. Looking at the error handling, the code creates aGitCommandErrorwith a static detail string and the command context, which aligns with the conventions since it's not just copying the cause message. I'm also checking the retry logic that inspects the error's_tagfield to determine if it's an "Unknown" platform error — that structural predicate check is appropriate for the retry condition. I need to adjust how the repo parameter is being formatted. I'm checking howgitCommandContextandGitCommandErrorwork together, particularly whether command arguments are being copied into error messages. Thedetailfield here is a static string rather than copying from the cause, which is fine. I should verify whethercommandContextincludesargsand howGitCommandErrorhandles them, though this appears to be an existing pattern rather than something new. Looking at the imports and checking how thewhilepredicate works withEffect.retry— the argument passed towhileis the error itself, and sincefileSystem.removereturns anEffect<void, PlatformError>, I'm working with aPlatformErrorthat has a_tagproperty. Now I'm reviewing the Effect Service Conventions to see if there are any violations in this diff. The imports look fine, there's no new service being defined, andparsePorcelainWorktreePathsis just a pure helper being exported. TheGitCommandErrorconstruction with a staticdetailstring and the cause looks correct — the convention says not to copycause.messageintodetail, and that's not what's happening here. The real question is whether spreadingcommandContextinto the error violates the convention about not leaking raw command arguments or output. But this pattern already exists throughout the file, so it's not something new introduced by this PR. Since the instruction says not to demand unrelated repository-wide cleanup, I should check whatgitCommandContextactually contains to understand if this is truly a violation or just existing practice. I'm checking the definition ofgitCommandContextto verify it properly sanitizes the arguments. ThegitCommandContextis properly sanitized to only expose safe fields like operation and command without leaking raw arguments, and the error construction handles the optionalcauseparameter correctly. I'm checking whether the retry predicate and error handling follow Effect service conventions, particularly around the use ofcatchIfwith structural predicates. TheLayer.provideMergechange in tests is justified since the test now needsServerConfigin scope, and injecting a flaky filesystem throughGitVcsDriver.makeis an acceptable test seam per the conventions. The focused tests on the new retry and filesystem pre-delete behavior are appropriate for a PR with actual behavior changes, not just mechanical refactoring. Now I'm checking whether the retry predicatecause.reason._tagaligns with how Effect'sPlatformError.SystemErroractually structures its reason field, and examining how the test constructs the transient failure error to verify the predicate will work correctly. There's a tension between the system prompt requiring JSON schema output and the customer instructions saying to output exactly "All clear" when there are no findings. Since I must produce JSON, I'll set the state to success with the title and summary reflecting that everything is clear.
Agent Credits: 47 credits