fix(install-targets): validate compiled OpenCode plugin before install#2041
Open
gaurav0107 wants to merge 4 commits into
Open
fix(install-targets): validate compiled OpenCode plugin before install#2041gaurav0107 wants to merge 4 commits into
gaurav0107 wants to merge 4 commits into
Conversation
Fixes affaan-m#2040. The OpenCode home installer copies plugin sources from `.opencode/` into the user's install root, but only `.opencode/dist/` (produced by `scripts/build-opencode.js`) contains the compiled JavaScript that OpenCode actually loads. From a plain `git clone`, that directory does not exist until `npm run build:opencode` (or `npm pack`/`prepack`) has run, so users who go straight to `node scripts/install-apply.js --target opencode --profile full` end up with `/` slash commands that show in autocomplete but silently do nothing — the plugin entry point loads, the runtime has no JS to execute. This change adds a `validate(input)` override on `opencode-home` that fails fast with an actionable message pointing at `node scripts/build-opencode.js` (or `npm run build:opencode`) when `.opencode/dist/index.js` is missing in the source repo. It does not change the install destination or any other runtime behaviour, and it has no effect when the plugin has already been built (the common case once `prepack` runs during `npm pack`). Tests: - `tests/lib/install-targets.test.js`: adds the previously-missing per-adapter resolution test for `opencode-home`, plus two `validate()` cases that cover the missing-build and built-payload code paths via temp `repoRoot` directories. - `node tests/run-all.js` is green (2571 passed, 0 failed). Docs: - `.opencode/README.md`: documents the build-first requirement under the existing "Direct Use" option so future users running the installer from a clone hit the build step before the validate gate fires.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a validator that requires a home directory and compiled OpenCode plugin payload under ChangesOpenCode Home Installation Validation
Sequence Diagram(s)sequenceDiagram
participant Installer as Installer (scripts/install-apply)
participant Validator as defaultValidateOpencodeHome
participant FS as FileSystem
Installer->>Validator: validate({ repoRoot, homeDir })
Validator->>FS: resolve and check repoRoot/.opencode/dist (index.js, plugins, tools)
FS-->>Validator: existence/type results
Validator-->>Installer: return validation issues[] or []
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 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 |
Contributor
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/lib/install-targets/opencode-home.js`:
- Around line 28-40: The current validation only checks compiledPluginPath
(using COMPILED_PLUGIN_RELATIVE_PATH) for .opencode/dist/index.js; update the
check in the function that returns buildValidationIssue to also verify the
existence of the runtime directories .opencode/dist/plugins and
.opencode/dist/tools (resolve them relative to input.repoRoot like
compiledPluginPath), collect any missing paths into a list and, if non-empty,
return buildValidationIssue('error','opencode-plugin-not-built', ...) with a
message describing all missing artifacts and include the missing paths in the
metadata so callers can see which specific files/directories failed validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61d4a2ac-ea97-469e-9d07-5649b5eb4370
📒 Files selected for processing (3)
.opencode/README.mdscripts/lib/install-targets/opencode-home.jstests/lib/install-targets.test.js
…encode CodeRabbit flagged that the opencode-home validate gate only checked `.opencode/dist/index.js`, but the OpenCode runtime contract also needs `.opencode/dist/plugins/` (plugin hooks) and `.opencode/dist/tools/` (custom tools) — both are produced by `scripts/build-opencode.js` and both are listed in `tests/scripts/build-opencode.test.js` as required npm-pack contents. A half-finished or interrupted compile that left only `index.js` would have squeezed past the previous gate and reproduced the original `/`-commands-do-nothing symptom from affaan-m#2040. The validate function now collects every missing required artefact into a single `error` issue with `missingRelativePaths` metadata so the maintainer (and any caller code) can see exactly which artefact triggered the failure. The error message lists all missing paths in one line and still points at `node scripts/build-opencode.js`. A new test case `opencode adapter validate reports a partial build (entry present, runtime dirs absent)` covers the half-built scenario, and the existing "passes once compiled plugin payload exists" test now materializes both subdirectories so it asserts the full happy path. Full suite still green: `node tests/run-all.js` → 2572 passed, 0 failed.
Contributor
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/lib/install-targets/opencode-home.js`:
- Around line 10-15: The current validation only uses existsSync and can accept
wrong entry types; update the install validation to verify types: for
COMPILED_PLUGIN_DIST_DIR and entries in REQUIRED_COMPILED_RELATIVE_PATHS ensure
index.js is a regular file and plugins and tools are directories by using
fs.statSync or fs.lstatSync and checking .isFile()/.isDirectory(); replace the
generic existsSync checks (references: COMPILED_PLUGIN_DIST_DIR,
REQUIRED_COMPILED_RELATIVE_PATHS, and the validation logic around the second
check block at lines ~33-38) with these explicit type checks and surface a clear
error when a path exists but has the wrong type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a28b12e-c68f-4148-b276-277f98b0ff8b
📒 Files selected for processing (2)
scripts/lib/install-targets/opencode-home.jstests/lib/install-targets.test.js
Per CodeRabbit: existsSync alone treats any filesystem entry as valid, which would let a stray file at `.opencode/dist/plugins` (or tools) squeeze past the gate even though OpenCode requires those entries to be directories. The validator now stat()s each required artefact and asserts `isFile()` for `dist/index.js` and `isDirectory()` for `dist/plugins` and `dist/tools`, returning the same `opencode-plugin-not-built` issue with an extra `expectedTypes` metadata array so callers can tell missing-vs-wrong- type failures apart. A new test materializes regular files where directories are expected and asserts the validator flags `dist/plugins` and `dist/tools` while leaving the correctly-typed `dist/index.js` alone. Existing happy-path and partial-build tests continue to pass. Full suite: 2573/2573 (one new test).
Contributor
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/lib/install-targets/opencode-home.js`:
- Around line 18-27: The isExpectedType function only treats ENOENT as a
non-error; change its error handling to also treat ENOTDIR as a validation miss
by returning false when error.code === 'ENOTDIR' instead of rethrowing, so
callers like validate and planInstallTargetScaffold receive a false result
rather than an exception; update the catch in isExpectedType to check for both
'ENOENT' and 'ENOTDIR' and return false for those cases, and rethrow other
errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42761317-f2db-4bfa-ade6-d510b8aefd25
📒 Files selected for processing (2)
scripts/lib/install-targets/opencode-home.jstests/lib/install-targets.test.js
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…lidate isExpectedType() previously rethrew every non-ENOENT filesystem error. When an intermediate path component is a regular file (e.g. .opencode/dist itself is a file rather than a directory), fs.statSync raises ENOTDIR — that escaped out of the validate gate and crashed planInstallTargetScaffold instead of producing the structured opencode-plugin-not-built issue. Now both ENOENT and ENOTDIR collapse to a 'missing artefact' return so the gate keeps producing the actionable error message. Other I/O errors (EACCES, EIO, ...) still surface — those are real system faults the user needs to see. Adds a regression test that materialises .opencode/dist as a regular file and asserts validate() does not throw and surfaces the structured issue.
Contributor
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
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
Fixes #2040.
OpenCode installs through
node scripts/install-apply.js --target opencode --profile fullend up with/slash commands visible in autocomplete butsilently non-functional — the plugin entry point loads, but
.opencode/dist/(the compiled JavaScript payload OpenCode actually executes) is missing from
fresh
git clonecheckouts becausescripts/build-opencode.jsonly runs aspart of
npm pack'sprepackhook. This PR makes that broken-state installfail fast with an actionable message instead of installing successfully and
leaving the user with a non-functional plugin surface.
Root cause
scripts/build-opencode.jsproduces.opencode/dist/{index.js, plugins/, tools/}.That dist is wired into
npm packviaprepackand thefilesarray inpackage.json, so npm-package consumers always get a working payload. Butthe installer (
scripts/install-apply.js → opencode-home) copies straight from.opencode/in the source repo and never checks for the compiled output. Afresh clone has no
dist/(it's gitignored), so the installer happily copiesTypeScript sources into
~/.opencode/and OpenCode then refuses to executethem.
The reporter's traceback confirms this exact symptom: commands appear in the
autocomplete list (because
commands/*.mddoes land), but pressing enter doesnothing (because plugin hooks/tools have no compiled JS to run).
Fix
scripts/lib/install-targets/opencode-home.jsnow declares avalidate(input)override that:
missing-home-dirissue when no home directory isresolvable (matches the helper's default behaviour for
kind: 'home').input.repoRootis provided, checks that<repoRoot>/.opencode/dist/index.jsexists. If it doesn't, returns a single
error-severity issue with codeopencode-plugin-not-builtwhose message names the missing path and theexact build command (
node scripts/build-opencode.js/npm run build:opencode).registry.planInstallTargetScaffoldalready aborts on anyerror-severityissue, so the user sees a clear, actionable failure instead of an apparently
successful install that breaks at runtime.
No public API changes. No change to install destinations, manifests, schemas,
or runtime behaviour for already-built repos.
Test plan
node tests/lib/install-targets.test.js— 38/38 passing (35 baseline + 3 new).opencode-home(matches the convention every other home adapter follows).
os.tmpdir()repoRoot.dist/index.jsandexpects
validateto return[].node tests/scripts/install-apply.test.js— 28/28 passing (no regression;the suite never invokes
--target opencode, so the new gate is opt-in).node tests/scripts/build-opencode.test.js— 3/3 passing.node tests/opencode-config.test.js— 3/3 passing.node tests/docs/ecc2-release-surface.test.js— 28/28 passing (assertsthe README content stays in the release surface manifest).
node tests/run-all.js— full suite, 2571/2571 passing.npx markdownlint-cli .opencode/README.md— exit 0.npx eslint scripts/lib/install-targets/opencode-home.js tests/lib/install-targets.test.js— clean.Out of scope
Open questions I noticed while researching but deliberately did not roll into
this PR — happy to follow up in separate changes if the maintainer agrees:
opencode-projectadapter mirroringclaude-project: would let usersinstall ECC into a project-local
.opencode/(parallel to the recently-added
claude-project). Useful but expands the surface (manifest, schema,request-validation), so kept out of this fix.
~/.opencodeto~/.config/opencode(the XDG path that upstream
sst/opencodeconstructs fromxdg-basedir):the issue reporter's "ATTEMP FIX" patches this. Worth noting that
Global.Path.home + .opencodeis in OpenCode's search order(
packages/opencode/src/config/paths.ts),so the existing path is not actually broken — just non-canonical. Changing
it is a behaviour shift for every existing install-state file and feels
like maintainer territory.
build-opencode.jsfrom inside the installer: wouldrequire
tscto be reachable on every install run, which regresses thefast-path. The validate gate keeps the toolchain boundary clean and lets
prepack/CI own the compile step.Summary by cubic
Fail fast when installing the OpenCode plugin from a fresh clone if the compiled payload is missing or the wrong type. The
opencode-hometarget now requires.opencode/dist/index.js(file),.opencode/dist/plugins/(dir), and.opencode/dist/tools/(dir), and tells users to build first.erroropencode-plugin-not-builtwith missing paths and expected types; hintsnode scripts/build-opencode.jsornpm run build:opencode; preservesmissing-home-dir; no change when already built..opencode/distis a file) as a missing artefact instead of throwing; other I/O errors still surface..opencode/README.md; tests cover adapter resolution, missing/partial builds, wrong types, ENOTDIR, and the built path.Written for commit 8475841. Summary will update on new commits. Review in cubic
Summary by CodeRabbit