Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
github-actions
bot
added
size:S
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| ), | ||
| Effect.result, | ||
| ); | ||
| if (Result.isFailure(pathExists) || pathExists.success === Option.none()) { |
There was a problem hiding this comment.
🔴 Critical Layers/ProviderCommandReactor.ts:270
When the workspace path does not exist, Effect.succeed(false) creates a Result.Success(false), but the condition pathExists.success === Option.none() never matches because false is not Option.none(). The missing-path error is therefore never raised. Consider checking Result.isSuccess(pathExists) && !pathExists.value to detect the non-existent path case.
Also found in 1 other location(s)
apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts:1705
The path existence check logic at line 276 is incorrect. The condition
pathExists.success === Option.none()will never match the case where the path doesn't exist. Whenfs.statfails withNotFound, the code returnsEffect.succeed(false), meaningpathExists.successwill beSome(false), notNone. ComparingSome(false) === Option.none()will always befalse(reference inequality). The check should verify the actual boolean value, e.g.,Option.isNone(pathExists.success) || !pathExists.success.valueor using the appropriateResultaccessor to check if the value isfalse.
🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/orchestration/Layers/ProviderCommandReactor.ts around line 270:
When the workspace path does not exist, `Effect.succeed(false)` creates a `Result.Success(false)`, but the condition `pathExists.success === Option.none()` never matches because `false` is not `Option.none()`. The missing-path error is therefore never raised. Consider checking `Result.isSuccess(pathExists) && !pathExists.value` to detect the non-existent path case.
Evidence trail:
1. apps/server/src/orchestration/Layers/ProviderCommandReactor.ts lines 262-277 (viewed at REVIEWED_COMMIT) - shows the pathExists logic and the condition check
2. package.json shows effect version 4.0.0-beta.43
3. Effect v4 Result.ts source (https://github.com/Effect-TS/effect-smol/blob/main/packages/effect/src/Result.ts) - confirms `Success` interface has `readonly success: A` (direct value, not Option-wrapped)
4. The condition `pathExists.success === Option.none()` at line 270 compares boolean to Option object, which never matches
Also found in 1 other location(s):
- apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts:1705 -- The path existence check logic at line 276 is incorrect. The condition `pathExists.success === Option.none()` will never match the case where the path doesn't exist. When `fs.stat` fails with `NotFound`, the code returns `Effect.succeed(false)`, meaning `pathExists.success` will be `Some(false)`, not `None`. Comparing `Some(false) === Option.none()` will always be `false` (reference inequality). The check should verify the actual boolean value, e.g., `Option.isNone(pathExists.success) || !pathExists.success.value` or using the appropriate `Result` accessor to check if the value is `false`.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b949ec7. Configure here.
| ), | ||
| Effect.result, | ||
| ); | ||
| if (Result.isFailure(pathExists) || pathExists.success === Option.none()) { |
There was a problem hiding this comment.
Path validation never triggers due to wrong comparison
High Severity
The condition pathExists.success === Option.none() always evaluates to false, completely defeating the workspace path validation. After Effect.result, pathExists.success is a boolean (true or false), not an Option. Comparing a boolean to Option.none() via === can never match. When a path doesn't exist, the catchTag produces Effect.succeed(false), so pathExists.success is false — but the check never catches it, and the stale path is still passed to the provider, reproducing the exact bug this PR intended to fix.
Reviewed by Cursor Bugbot for commit b949ec7. Configure here.
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. Multiple reviewers have identified a critical bug in the implementation: the path existence check never triggers because it compares a boolean to an Option, which always fails. The fix as written does not work and requires correction before merge. You can customize Macroscope's approvability policy. Learn more. |
1 participant
What Changed
Added workspace path validation in
ProviderCommandReactor.tsthat checks if the project directory exists before attempting to start a provider session. When the path doesn't exist, returns a clear error message instead of the misleading "Claude Code native binary not found" error.Why
Bug #1726: When users rename a project directory via terminal/file explorer and then try to send a message to Claude, the error message was misleading:
Root cause: The database stores the original workspace path, and when the directory is renamed externally, the stale path is passed to the Claude SDK which fails with a confusing error about the binary not being found.
Checklist
Note
Low Risk
Low risk: adds a preflight filesystem existence check before session start and a test to lock the behavior in. Main risk is edge cases around filesystem stat errors/permissions affecting turn start.
Overview
Improves error handling when starting a turn for projects whose workspace directory was moved/deleted.
ProviderCommandReactornow validates the resolved workspacecwdexists before starting a provider session, and returns aProviderAdapterRequestErrorwith a clear, actionable message when it doesn’t.Adds a regression test asserting that a missing
workspaceRootfails with the new message and does not callstartSession/sendTurn.Reviewed by Cursor Bugbot for commit b949ec7. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Return a clear error when a project workspace path does not exist before starting a provider session
Before starting or resuming a provider session in
ProviderCommandReactor, a precondition check now stats the resolved workspace path usingFileSystem. If the path is missing or the stat fails, the reactor returns aProviderAdapterRequestErrorwith a message indicating the path no longer exists, and the session start is skipped.'no longer exists'and thatstartSessionis not called.📊 Macroscope summarized b949ec7. 2 files reviewed, 3 issues evaluated, 1 issue filtered, 1 comment posted
🗂️ Filtered Issues
apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts — 0 comments posted, 1 evaluated, 1 filtered
pathExists.success === Option.none()will never match the case where the path doesn't exist. Whenfs.statfails withNotFound, the code returnsEffect.succeed(false), meaningpathExists.successwill beSome(false), notNone. ComparingSome(false) === Option.none()will always befalse(reference inequality). The check should verify the actual boolean value, e.g.,Option.isNone(pathExists.success) || !pathExists.success.valueor using the appropriateResultaccessor to check if the value isfalse. [ Cross-file consolidated ]