fix: use file URL for dynamic imports to support Windows#156
Merged
Conversation
Node's ESM dynamic `import()` rejects bare absolute paths on Windows because the drive letter (e.g. `C:`) is interpreted as an unsupported URL scheme. Wrap the path with `pathToFileURL()` so it works cross-platform. Fixes #155
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds platform detection and SPAWN_OPTS for cross-platform child-process handling, migrates browser-launch from ChangesPlatform & runtime behavior updates
Browser launcher migration
CRLF-tolerant env parsing
Registry import and metadata
Possibly related PRs:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
Greptile SummaryThis PR fixes Windows incompatibilities in the WorkOS CLI by addressing ESM dynamic import path handling, child-process spawning, signal handling, and
Confidence Score: 5/5Safe to merge — all changes are targeted Windows compatibility fixes with no regressions on existing POSIX behavior. The changes are narrow and well-scoped: the ESM URL fix, the opn→open migration, the SPAWN_OPTS abstraction, and the CRLF normalisation all address confirmed Windows failure modes. Each fix is independently verifiable, the existing test suite passes (1800 tests), and the author validated the end-to-end flow on Windows 11. Non-Windows code paths are unaffected because SPAWN_OPTS expands to shell:false (the Node.js default) on POSIX, and all other changes are either additive guards or standard library swaps. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "chore: formatting" | Re-trigger Greptile |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
- Add cross-platform spawn helper (shell: true on Windows for .cmd shims) - Fix URL.pathname in bin.ts (use fileURLToPath for Windows drive letters) - Fix node_modules/.bin resolution to prefer .cmd on Windows - Fix all env file parsers to handle CRLF line endings - Replace opn with open for better Windows URL handling - Add SIGBREAK handler for Windows signal handling - Fix Prettier step to use spawn with array args (safe path handling) - Fix package install to use spawn with array args (safe special chars) - Fix Gradle wrapper resolution for Windows (gradlew.bat) - Fix Goose agent config dir to use %APPDATA% on Windows - Fix vercel spawn calls with shell option Ref #155
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a9d8d0f-beba-4d8c-9411-9943bdb85fd9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
package.jsonsrc/bin.tssrc/commands/claim.spec.tssrc/commands/claim.tssrc/commands/dev.tssrc/commands/emulate.tssrc/commands/install-skill.tssrc/commands/login.spec.tssrc/commands/login.tssrc/doctor/checks/auth-patterns.tssrc/doctor/checks/environment.tssrc/lib/credential-discovery.tssrc/lib/dev-command.tssrc/lib/run-with-core.tssrc/lib/validation/build-validator.tssrc/lib/validation/quick-checks.tssrc/lib/validation/validator.tssrc/steps/run-prettier.tssrc/steps/upload-environment-variables/providers/vercel.tssrc/utils/clack-utils.tssrc/utils/env-parser.tssrc/utils/exec-file.tssrc/utils/platform.ts
✅ Files skipped from review due to trivial changes (3)
- package.json
- src/lib/credential-discovery.ts
- src/lib/validation/validator.ts
Comment on lines
+337
to
+345
| fs.writeFileSync( | ||
| join(process.cwd(), `workos-installation-error-${Date.now()}.log`), | ||
| JSON.stringify({ | ||
| stdout: redactSensitiveInfo(stdout), | ||
| stderr: redactSensitiveInfo(stderr), | ||
| }), | ||
| { encoding: 'utf8' }, | ||
| ); | ||
| reject(new Error(`${cmd} exited with code ${code}\n${stderr}`)); |
There was a problem hiding this comment.
Don't let error-log persistence crash the failure path.
If fs.writeFileSync(...) throws here, the 'close' handler will throw asynchronously and the install promise won't reject with the original package-manager error. Please make the log write best-effort and non-blocking before rejecting.
Proposed fix
- proc.on('close', (code) => {
+ proc.on('close', async (code) => {
if (code !== 0) {
- fs.writeFileSync(
- join(process.cwd(), `workos-installation-error-${Date.now()}.log`),
- JSON.stringify({
- stdout: redactSensitiveInfo(stdout),
- stderr: redactSensitiveInfo(stderr),
- }),
- { encoding: 'utf8' },
- );
+ try {
+ await fs.promises.writeFile(
+ join(process.cwd(), `workos-installation-error-${Date.now()}.log`),
+ JSON.stringify({
+ stdout: redactSensitiveInfo(stdout),
+ stderr: redactSensitiveInfo(stderr),
+ }),
+ { encoding: 'utf8' },
+ );
+ } catch (error) {
+ debug('Failed to write installation error log:', error);
+ }
reject(new Error(`${cmd} exited with code ${code}\n${stderr}`));
} else {
resolve();
}
});
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
1 participant
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
import()path inregistry.tswithpathToFileURL().hrefso integration modules load on WindowsC:\Users\...\index.jsfail withERR_UNSUPPORTED_ESM_URL_SCHEMEbecause Node ESM interprets the drive letter as a URL schemetry/catchsilently swallowed the error, leaving the registry empty and producing "Could not detect framework integration"Fixes #155
Test plan
pnpm buildpassespnpm typecheckpassespnpm testpasses (all 1800 tests)npx workosin a Next.js project detects the frameworkSummary by CodeRabbit