Repository navigation
fix(setup): remove stale CJS bundle check from setup-open-code - #3908
diegosouzapw merged 1 commit into
Conversation
PR diegosouzapw#3883 dropped the CJS bundle from tsup (format: ["esm"] only), but bin/cli/commands/setup-open-code.mjs::resolveBundledPlugin() still checks for dist/index.cjs: if (!existsSync(esmEntry) || !existsSync(cjsEntry)) { throw ... } The cjs check always fails now, so the setup command throws even when the plugin is correctly built (only the ESM dist is present). This breaks `omniroute setup opencode` on the release branch post-diegosouzapw#3883. Drop the cjsEntry check, update the JSDoc return type, and refresh the comment to reflect the ESM-only build. All 8 existing tests pass.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request transitions the @omniroute/opencode-plugin build to ESM-only by removing CommonJS (CJS) support in bin/cli/commands/setup-open-code.mjs. The review feedback correctly points out that the JSDoc description still references the deleted dist/index.cjs file and needs updating. Additionally, the feedback notes that corresponding tests must be added or updated to comply with the repository style guide regarding changes to production code under the bin/ directory.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| * may not have tsup available (it's a devDependency). | ||
| * | ||
| * @returns {{ distEntry: string, cjsEntry: string, packageDir: string }} | ||
| * @returns {{ distEntry: string, packageDir: string }} |
There was a problem hiding this comment.
The JSDoc description above (specifically on line 84 of the file) still mentions dist/index.cjs (e.g., Built (dist/index.cjs + dist/index.js exist)). Since the CommonJS bundle has been dropped, please update that description to reflect that only dist/index.js is expected.
| const cjsEntry = join(BUNDLED_PLUGIN_DIR, "dist", "index.cjs"); | ||
|
|
||
| if (!existsSync(esmEntry) || !existsSync(cjsEntry)) { | ||
| if (!existsSync(esmEntry)) { |
There was a problem hiding this comment.
According to the Repository Style Guide (Rule 9), you must always include or update tests when changing production code under bin/. Since bin/cli/commands/setup-open-code.mjs has been modified, please ensure that corresponding test changes or additions are included in this pull request.
References
- Always include tests when changing production code (src/, open-sse/, electron/, bin/). (link)
|
Thanks, @herjarsa! 🙏 Perfect follow-up to #3883 — after the CJS bundle was dropped, |
…souzapw#3908) Integrated into release/v3.8.26
2 participants
Follow-up to #3883.
#3883 dropped the CJS bundle from tsup (
format: ["esm"]only), butbin/cli/commands/setup-open-code.mjs::resolveBundledPlugin()still checks fordist/index.cjs:The cjs check always fails now, so
omniroute setup opencodethrows even when the plugin is correctly built (only the ESM dist is present). This breaks the setup command on the release branch post-#3883.Changes
cjsEntrycheck inresolveBundledPlugin()@returnsto removecjsEntryTesting
tests/unit/cli-setup-opencode.test.tspasstests/unit/cli-setup-command.test.tspassnpm run typecheck:corecleanScoped narrowly to the CJS check; the other follow-ups you mentioned (security validation, Windows USERPROFILE) are skipped here since they touched the same file and would conflict with the version on the release branch. Happy to rebase against a more recent base if you want those too.