fix(opencode-plugin): drop CJS bundle to fix OpenCode plugin loader - #3883
diegosouzapw merged 1 commit into
Conversation
![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 bundles the @omniroute/opencode-plugin pre-built inside the omniroute npm package and introduces a new CLI command omniroute setup opencode to automate its installation and registration within OpenCode. It also drops the CommonJS (CJS) build format in favor of ESM-only, fixes baseURL resolution fallbacks for partner/tiered providers, and adds comprehensive unit tests for the new setup command. Review feedback identifies several critical issues, including a runtime crash in the setup command due to checking for the deleted CJS bundle, a platform-specific test sandbox issue on Windows that could overwrite real user configurations, a potential command injection vulnerability on Windows, and several unused variables that should be cleaned up.
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.
| const esmEntry = join(BUNDLED_PLUGIN_DIR, "dist", "index.js"); | ||
| const cjsEntry = join(BUNDLED_PLUGIN_DIR, "dist", "index.cjs"); | ||
|
|
||
| if (!existsSync(esmEntry) || !existsSync(cjsEntry)) { | ||
| throw new Error( | ||
| `@omniroute/opencode-plugin dist/ not built (looked for ${esmEntry}).\n` + | ||
| `Run \`cd ${BUNDLED_PLUGIN_DIR} && npm install && npm run build\` and re-run this command.` | ||
| ); | ||
| } | ||
|
|
||
| // Prefer ESM. OpenCode (≥1.15) loads ESM modules natively. | ||
| return { distEntry: esmEntry, cjsEntry, packageDir: BUNDLED_PLUGIN_DIR }; |
There was a problem hiding this comment.
Since the CJS bundle has been dropped in this PR (tsup now only builds the ESM format), dist/index.cjs will no longer exist. The check !existsSync(esmEntry) || !existsSync(cjsEntry) will always fail, causing the setup command to throw an error and prevent installation. We should remove the check for cjsEntry entirely.
| const esmEntry = join(BUNDLED_PLUGIN_DIR, "dist", "index.js"); | |
| const cjsEntry = join(BUNDLED_PLUGIN_DIR, "dist", "index.cjs"); | |
| if (!existsSync(esmEntry) || !existsSync(cjsEntry)) { | |
| throw new Error( | |
| `@omniroute/opencode-plugin dist/ not built (looked for ${esmEntry}).\n` + | |
| `Run \`cd ${BUNDLED_PLUGIN_DIR} && npm install && npm run build\` and re-run this command.` | |
| ); | |
| } | |
| // Prefer ESM. OpenCode (≥1.15) loads ESM modules natively. | |
| return { distEntry: esmEntry, cjsEntry, packageDir: BUNDLED_PLUGIN_DIR }; | |
| const esmEntry = join(BUNDLED_PLUGIN_DIR, "dist", "index.js"); | |
| if (!existsSync(esmEntry)) { | |
| throw new Error( | |
| `@omniroute/opencode-plugin dist/ not built (looked for ${esmEntry}).\n` + | |
| `Run \`cd ${BUNDLED_PLUGIN_DIR} && npm install && npm run build\` and re-run this command.` | |
| ); | |
| } | |
| // Prefer ESM. OpenCode (≥1.15) loads ESM modules natively. | |
| return { distEntry: esmEntry, packageDir: BUNDLED_PLUGIN_DIR }; |
| async function withSandbox(fn: (sandbox: string, homeDir: string) => Promise<void>) { | ||
| const sandbox = createTempDir(); | ||
| // Create a fake HOME so opencode.json goes to sandbox/.config/opencode/ | ||
| const fakeHome = path.join(sandbox, "fake-home"); | ||
| fs.mkdirSync(fakeHome, { recursive: true }); | ||
| const oldHome = process.env.HOME; | ||
| process.env.HOME = fakeHome; | ||
| // Ensure XDG_CONFIG_HOME is not set so resolveOpenCodeDirs uses HOME | ||
| delete process.env.XDG_CONFIG_HOME; | ||
|
|
||
| try { | ||
| await fn(sandbox, fakeHome); | ||
| } finally { | ||
| process.env.HOME = oldHome; | ||
| if (ORIGINAL_XDG_CONFIG_HOME === undefined) { | ||
| delete process.env.XDG_CONFIG_HOME; | ||
| } else { | ||
| process.env.XDG_CONFIG_HOME = ORIGINAL_XDG_CONFIG_HOME; | ||
| } | ||
| fs.rmSync(sandbox, { recursive: true, force: true }); | ||
| } | ||
| } |
There was a problem hiding this comment.
On Windows, os.homedir() resolves using the USERPROFILE environment variable rather than HOME. Overriding only process.env.HOME in the test sandbox means that on Windows, the test will read and write to the developer's actual home directory, potentially corrupting or overwriting their real opencode.json configuration. We must override and restore process.env.USERPROFILE as well.
async function withSandbox(fn: (sandbox: string, homeDir: string) => Promise<void>) {
const sandbox = createTempDir();
// Create a fake HOME so opencode.json goes to sandbox/.config/opencode/
const fakeHome = path.join(sandbox, "fake-home");
fs.mkdirSync(fakeHome, { recursive: true });
const oldHome = process.env.HOME;
const oldUserProfile = process.env.USERPROFILE;
process.env.HOME = fakeHome;
process.env.USERPROFILE = fakeHome;
// Ensure XDG_CONFIG_HOME is not set so resolveOpenCodeDirs uses HOME
delete process.env.XDG_CONFIG_HOME;
try {
await fn(sandbox, fakeHome);
} finally {
process.env.HOME = oldHome;
if (oldUserProfile === undefined) {
delete process.env.USERPROFILE;
} else {
process.env.USERPROFILE = oldUserProfile;
}
if (ORIGINAL_XDG_CONFIG_HOME === undefined) {
delete process.env.XDG_CONFIG_HOME;
} else {
process.env.XDG_CONFIG_HOME = ORIGINAL_XDG_CONFIG_HOME;
}
fs.rmSync(sandbox, { recursive: true, force: true });
}
}| export async function runSetupOpenCodeCommand(opts = {}) { | ||
| const providerId = opts.providerId || "omniroute"; | ||
| const baseURL = opts.baseURL || "http://localhost:20128"; | ||
| const displayName = opts.displayName || null; | ||
| const wantsAuth = Boolean(opts.auth); | ||
| const nonInteractive = Boolean(opts.nonInteractive); |
There was a problem hiding this comment.
To prevent potential command injection on Windows (where spawnSync is run with shell: true) and directory traversal issues, we should validate that providerId only contains safe alphanumeric characters, dashes, or underscores. Additionally, validating that baseURL is a well-formed URL prevents writing invalid configurations to opencode.json.
export async function runSetupOpenCodeCommand(opts = {}) {
const providerId = opts.providerId || "omniroute";
const baseURL = opts.baseURL || "http://localhost:20128";
const displayName = opts.displayName || null;
const wantsAuth = Boolean(opts.auth);
const nonInteractive = Boolean(opts.nonInteractive);
if (!/^[a-zA-Z0-9-_]+$/.test(providerId)) {
printError("Invalid provider ID. Only alphanumeric characters, dashes, and underscores are allowed.");
return { exitCode: 1 };
}
try {
new URL(baseURL);
} catch {
printError("Invalid base URL format.");
return { exitCode: 1 };
}| function registerPluginInOpenCodeConfig({ | ||
| opencodeConfigDir, | ||
| pluginTargetDir, | ||
| providerId, | ||
| baseURL, | ||
| displayName, | ||
| }) { |
There was a problem hiding this comment.
The parameter pluginTargetDir is defined in registerPluginInOpenCodeConfig but is never used within the function body. We should remove it to keep the code clean.
function registerPluginInOpenCodeConfig({
opencodeConfigDir,
providerId,
baseURL,
displayName,
}) {| const reg = registerPluginInOpenCodeConfig({ | ||
| opencodeConfigDir, | ||
| pluginTargetDir, | ||
| providerId, | ||
| baseURL, | ||
| displayName, | ||
| }); |
There was a problem hiding this comment.
Remove the unused pluginTargetDir argument from the call to registerPluginInOpenCodeConfig.
const reg = registerPluginInOpenCodeConfig({
opencodeConfigDir,
providerId,
baseURL,
displayName,
});| const ORIGINAL_HOME = process.env.HOME; | ||
| const ORIGINAL_XDG_CONFIG_HOME = process.env.XDG_CONFIG_HOME; |
There was a problem hiding this comment.
The global constant ORIGINAL_HOME is declared but never used. We should remove it.
| const ORIGINAL_HOME = process.env.HOME; | |
| const ORIGINAL_XDG_CONFIG_HOME = process.env.XDG_CONFIG_HOME; | |
| const ORIGINAL_XDG_CONFIG_HOME = process.env.XDG_CONFIG_HOME; |
OpenCode v1.17.x's Bun-based plugin loader resolves the package main/exports and applies CJS-to-ESM interop on the dual CJS bundle, so mod.default becomes the whole exports namespace instead of the V1 plugin object. V1 detection then fails and the legacy path iterates the namespace and throws 'Plugin export is not a function' — the OmniRoute provider never registers. Ship ESM-only (format: [esm], cjsInterop: false) with main -> ./dist/index.js and a ./runtime subpath export, matching the predecessor opencode-omniroute-auth package that loaded correctly. Adds a regression test asserting the ESM-only package.json/tsup shape so a CJS bundle can't be re-introduced. Scoped down to just the loader fix; the CLI setup-opencode + baseURL changes from the original PR are deferred to a separate follow-up. Co-authored-by: diegosouzapw <diegosouza.pw@gmail.com>
5ac782b to
7c6ce30
Compare
|
Thanks, @herjarsa — and great root-cause writeup. 🙏 The diagnosis is spot-on: OpenCode v1.17.x's Bun loader applies CJS-to-ESM interop on the dual bundle, so To land this quickly and safely, I scoped the PR down to just the loader fix ( A couple of the other changes I deliberately left out of this merge, so they can be handled separately:
Really appreciate the fix — this unblocks the OmniRoute provider in OpenCode v1.17.x. 🚀 |
|
Added 3 follow-up commits addressing the bot review comments + an additional fix:
All 6 existing tests pass; typecheck clean. The branch was force-pushed to herjarsa/OmniRoute @ 512224c. |
|
Thanks for the careful review and the scoped merge \ud83d\ude4f \ud83d\ude80 You're right on both counts:
Closing this discussion here \u2014 thanks for the quick turnaround on the loader fix! |
The plugin became ESM-only when the CJS bundle was dropped to fix the OpenCode loader (#3883), so tests/scaffold.test.ts's 'CJS default export resolves via require()' test fails at publish time with 'Cannot find module ../dist/index.cjs' (it only runs in the npm-publish opencode-plugin job, so the cycle never caught it). Replaced with an ESM import of the built dist/index.js asserting the same v1 { id, server } shape; dropped the now-unused createRequire import. omniroute@3.8.26 itself already published fine.
The plugin became ESM-only when the CJS bundle was dropped to fix the OpenCode loader (diegosouzapw#3883), so tests/scaffold.test.ts's 'CJS default export resolves via require()' test fails at publish time with 'Cannot find module ../dist/index.cjs' (it only runs in the npm-publish opencode-plugin job, so the cycle never caught it). Replaced with an ESM import of the built dist/index.js asserting the same v1 { id, server } shape; dropped the now-unused createRequire import. omniroute@3.8.26 itself already published fine.
The plugin became ESM-only when the CJS bundle was dropped to fix the OpenCode loader (diegosouzapw#3883), so tests/scaffold.test.ts's 'CJS default export resolves via require()' test fails at publish time with 'Cannot find module ../dist/index.cjs' (it only runs in the npm-publish opencode-plugin job, so the cycle never caught it). Replaced with an ESM import of the built dist/index.js asserting the same v1 { id, server } shape; dropped the now-unused createRequire import. omniroute@3.8.26 itself already published fine.
…iegosouzapw#3883) Integrated into release/v3.8.26 (scoped to the ESM-only loader fix)
The plugin became ESM-only when the CJS bundle was dropped to fix the OpenCode loader (diegosouzapw#3883), so tests/scaffold.test.ts's 'CJS default export resolves via require()' test fails at publish time with 'Cannot find module ../dist/index.cjs' (it only runs in the npm-publish opencode-plugin job, so the cycle never caught it). Replaced with an ESM import of the built dist/index.js asserting the same v1 { id, server } shape; dropped the now-unused createRequire import. omniroute@3.8.26 itself already published fine.
2 participants
Summary
The
@omniroute/opencode-pluginv0.1.0 ships a dual ESM+CJS bundle via tsup, but OpenCode's Bun-based plugin loader resolves the package'smainfield (which points to the CJS bundle). When Bun applies CJS-to-ESM interop, the resultingmod.defaultbecomes the full exports namespace (not the V1 plugin object), soreadV1Pluginin OpenCode's loader fails the V1 detection and the loader falls through togetLegacyPlugins, which iteratesObject.values(mod)and chokes on the many named exports with:Net effect: the plugin fails to register in OpenCode v1.17.x and the OmniRoute provider never appears in the model picker.
Fix
Drop the CJS bundle; ship ESM only, with a
./runtimesubpath export — matching the pattern used by the predecessoropencode-omniroute-authpackage that worked correctly.Changes
@omniroute/opencode-plugin/tsup.config.ts@omniroute/opencode-plugin/package.jsonWhy this happens (root cause)
OpenCode's plugin loader does
import(entry)on the resolved file and uses Bun's CJS-interop on the dual-bundle CJS file.mod.defaultends up being the entire exports namespace, not the V1 plugin object.readV1Plugin(mod, spec, 'server', 'detect')does the V1 detection but the namespace lacks the V1id/servertop-level keys, so it falls through togetLegacyPlugins, which iterates the namespace and throws on the first non-function export.With the ESM-only fix, the loader imports
./dist/index.jsdirectly.mod.defaultis{ id, server: <function> }— V1 detection succeeds.Note on related OpenCode bug
The same
Plugin export is not a functionerror fires forsuperpowers@0.0.2and any other plugin with named exports. The fix landed upstream in commitanomalyco/opencode@2e27403b26e9795d5ab60ec2b71585aa540abfd0but is not in OpenCode 1.17.7. Tracking upstream: anomalyco/opencode#13543.Verification
After the fix, with the rebuilt plugin installed:
mod.defaultis the V1 object withserver: functionfailed to load pluginerrors;service=omniroutelog lines appearomnirouteprovider shows live catalog + combosnpm testgreencc @diegosouzapw