You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Here's my review of PR "Fix Windows setup and skills install flow":
Issues Found
1. Potentially redundant exitCode check in DiscoverPlugins.tsx
The action handlers pluginsApi.install(), .update(), .remove() all pass through unwrapBridgeResult which strips the exitCode field — on success they return undefined. The result?.exitCode check will always be undefined, so the "failed" branch via exit code is dead code. The catch block (thrown error) is the only way failures are detected.
If exit-code-based success signaling is intended, unwrapBridgeResult / the bridge handlers would need to propagate exitCode in the data payload. Otherwise this branch is unreachable and should be simplified to just treat any non-thrown return as success.
Severity: Low — not a bug, but misleading. The catch block correctly catches failures.
2. Windows root-directory edge case in cwd fallback
The cwd === "/" check guards against Linux root but doesn't cover Windows drive roots (e.g., C:\). If cwd is ever a bare Windows drive root, spawn could fail or behave unexpectedly.
Severity: Low — unlikely in practice since the app's cwd management probably doesn't produce bare drive roots on Windows.
No issues found in:
server/web-server.ts — isWithinAllowedRoot using path.relative is a correct drop-in replacement that properly handles Windows backslashes and eliminates false prefix matches.
src/App.tsx — suppressBootErrors is cleanly wired; the effect dependency array is complete and the guard prevents toasts during onboarding.
src/components/SetupWizard.tsx — Scrollable, closable card with proper responsive padding; close button correctly calls onComplete and cleans up state.
opencode-bridge.ts — npx.cmd, shell: process.platform === "win32", and homedir() fallback are all correct patterns for Windows compatibility.
Summary
Well-structured, focused PR. The Windows path fix (isWithinAllowedRoot) is the most important change, properly fixing the slash-prefix matching bug. The remaining changes are sensible UX improvements with no correctness issues.
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
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
Verification