fix(adapters): delete the two install.sh scripts instead of documenting them - #395
Conversation
…ting them as the install path `adapters/codex/install.sh` and `adapters/opencode/install.sh` were tracked as 100644, so the invocation every adapter README gave - `./adapters/codex/install.sh` - failed with "Permission denied" on any fresh clone. Both carry a bash shebang and are meant to be run directly, so the executable bit is what they should have had; they are the only shell scripts in the repo without it. The documented path was wrong in a second way: both scripts have been marked [DEPRECATED] in their own header since v5, because they install pre-rename plugin names, while `agentsys --tool codex` and `agentsys --tool opencode` are the supported installers. The three adapter READMEs led with the deprecated script in Quick Install, Updating, and Path errors alike. Those now show the CLI, with one note per adapter that the script still exists but is deprecated.
![revuto-review[bot]](https://avatars.githubusercontent.com/u/55848801?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is an auto review done by revuto.
Summary
The direction of this PR is right: agentsys --tool <tool> is the supported installer, and the [DEPRECATED] shell scripts should not be the first thing a reader is steered to. The chmod +x (mode-only) change and the Quick Install / Updating / Path-errors rewrites all check out, and the adapter READMEs are hand-maintained (no <!-- GEN: --> markers; gen-adapters.js:217 explicitly excludes README.md from generated/orphan handling), so nothing here collides with scripts/generate-docs.js or gen-adapters --check.
One substantive problem: the new note added in three places states the scripts "still work from a clone". They do not — they install zero commands/skills on any clone of this repo at HEAD. Details inline.
Verification notes (no action needed)
- No
CHANGELOG.mdlink definition required — no release files touched. __tests__/opencode-compatibility.test.js:255-328only asserts oninstall.shcontents, so neither the mode flip nor the README edits can break it.validate-cross-platform-docs.jsreadsadapters/{codex,opencode}/README.md, but itsnpm install -g agentsyscheck (line 287) only applies toREADME.md/docs/INSTALLATION.md/docs/CROSS_PLATFORM.md, so the adapter READMEs are unaffected either way.
| The `adapters/codex/install.sh` script still works from a clone, but it is | ||
| deprecated: it installs pre-rename plugin names. |
There was a problem hiding this comment.
This is an auto review done by revuto.
This note is inaccurate in the part that matters most: the script does not "still work from a clone" — it installs nothing at all, and the failure is silent.
Every source path in the script is rooted at the deleted plugins/ tree:
adapters/codex/install.sh:105—SOURCE_FILE="$REPO_ROOT/plugins/$PLUGIN/commands/$SOURCE_NAME.md"adapters/codex/install.sh:142—SOURCE_SKILL_DIR="$REPO_ROOT/plugins/$PLUGIN/skills/$SOURCE_SKILL"
and plugins/ is gone from this repo. git ls-tree -r HEAD -- plugins returns nothing, and docs/ARCHITECTURE.md:76 states it outright: "NOTE: plugins/ has been removed. All 24 plugins are now standalone repos … The installer fetches them from GitHub at install time." Nothing in a clone materializes it — no submodule (.gitmodules has only agent-knowledge), no prepare/postinstall hook in package.json, and bin/cli.js fetches plugins to the ~/.agentsys/plugins cache (getPluginCacheDir, line 301), not into the checkout.
So on a fresh clone every [ -f "$SOURCE_FILE" ] at line 109 fails and each mapping takes the [WARN] Skipped … (source not found: …) branch (line 127). The script then still prints [OK] Installation complete! (line 238) and enumerates seven skills it never wrote (lines 240-247) — a false success report, and set -e won't catch it because nothing returns non-zero.
Worse, the run is not a no-op: lines 171-177 rm -rf any existing deslop/review/reality-check-set/pr-merge/review-orchestrator skill dirs, and lines 180-184 rm -rf "$CODEX_CONFIG_DIR/prompts". Following this note can therefore remove content from a working ~/.codex while installing nothing back.
Given that, the chmod +x half of the PR cuts against the goal: it lowers the barrier to running a script that can only destroy state. Consider either wording the note to match reality (e.g. "the script is non-functional since plugins/ moved to standalone repos and will be removed") or, more simply, deleting the two scripts and the notes entirely — no test asserts they run, and bin/cli.js is the only working path. Same wording appears at adapters/opencode/README.md:12-13 and adapters/README.md:29-30 and :53-54.
| The `adapters/opencode/install.sh` script still works from a clone, but it is | ||
| deprecated: it installs pre-rename plugin names. |
There was a problem hiding this comment.
This is an auto review done by revuto.
Same inaccuracy as adapters/codex/README.md:12-13. adapters/opencode/install.sh:113 resolves every command from $REPO_ROOT/plugins/$PLUGIN/commands/$SOURCE_NAME.md, which no longer exists in a clone, so all seven COMMAND_MAPPINGS fall into the [WARN] Skipped /$TARGET_NAME (source not found: …) branch at line 122 while line 301 still prints [OK] Installation complete! and lines 304-307 list every command as installed. Line 127-133 also deletes pr-merge.md on the way through. The script does not "still work from a clone."
| The `adapters/codex/install.sh` script still works from a clone, but it is | ||
| deprecated: it installs pre-rename plugin names. |
There was a problem hiding this comment.
This is an auto review done by revuto.
Same inaccurate claim here (and again at lines 53-54 for OpenCode) — both install.sh scripts read from the removed plugins/ tree (adapters/codex/install.sh:105, adapters/opencode/install.sh:113) and install nothing from a clone while reporting success. If the notes stay, they should say the scripts are non-functional rather than "still works from a clone"; see the detailed comment on adapters/codex/README.md.
…ng them The previous commit made these executable and labeled them a deprecated fallback that "still works from a clone". It does not work, and lowering the barrier to running it is the wrong direction. Both scripts source every command and skill from $REPO_ROOT/plugins/, a tree that was removed when the plugins moved to standalone repos. Every mapping therefore misses its source file and takes the [WARN] Skipped branch; set -e catches nothing, and the script closes with "[OK] Installation complete!" followed by a list of seven skills it never wrote. Worse, it is not a no-op: before copying it rm -rf's five skill directories and ~/.codex/prompts, so running it deletes a working install and reports success. bin/cli.js (agentsys --tool codex|opencode) is the only install path, so the scripts go, along with the notes pointing at them. The 'Install script validation' block in the OpenCode compatibility suite asserted only on the dead script's contents - the XDG_CONFIG_HOME semantics it covered are exercised against the live resolver in dev-install.test.js. Also drops install.sh from gen-adapters' orphan exclusion set, which only ever tests .md and .json names.
avifenesh
changed the title
There was a problem hiding this comment.
This is an auto review done by revuto.
Reviewed the deletion of both install.sh scripts and the doc/test fallout. The core change checks out: bin/cli.js (installForCodex at bin/cli.js:1669, installForOpenCode at bin/cli.js:1501) is a real, working install path, the deleted scripts did resolve everything from the now-absent plugins/ tree while rm -rfing skill dirs first, the removed Jest block only asserted on that dead script's text (XDG_CONFIG_HOME empty/whitespace semantics are still covered by __tests__/dev-install.test.js:123-137), and dropping install.sh from EXCLUDED_FILES in findOrphanedAdapters is inert because the scan filters on .md/.json only (scripts/gen-adapters.js:229). One factual nit below on the replacement wording.
| ## Common Pitfalls | ||
|
|
||
| - **Don't forget to update both installers** - `bin/cli.js` AND `adapters/opencode/install.sh` | ||
| - **`bin/cli.js` is the only installer** - plugin changes land nowhere until it copies them |
There was a problem hiding this comment.
This is an auto review done by revuto.
bin/cli.js is not the only thing that copies this plugin. scripts/dev-install.js:399-412 has its own independent copy of the same logic:
const pluginSrcDir = path.join(SOURCE_DIR, 'adapters', 'opencode-plugin');
...
fs.copyFileSync(srcPath, destPath); // -> <opencode>/plugins/agentsys.tsIt is reachable via agentsys-dev dev-install opencode (bin/dev-cli.js:213-229), and it also duplicates the legacy awesome-slash.ts cleanup that bin/cli.js:1528-1531 does. So the original pitfall — two copy paths that can drift — still applies; only the second path changed identity (adapters/opencode/install.sh → scripts/dev-install.js). Suggest pointing the bullet (and the "Files to Update" table above, which lost its second row) at scripts/dev-install.js rather than asserting a single installer. The same "the only installer" phrasing was added in adapters/README.md:236.
…plugin too Review finding on this PR: replacing the deprecated-install.sh notes with "bin/cli.js is the only installer" overstated it. scripts/dev-install.js:399-412 copies adapters/opencode-plugin/index.ts to <opencode>/plugins/agentsys.ts with its own copy of the same logic, including the legacy awesome-slash.ts cleanup bin/cli.js:1528-1531 does, and it is reachable via `agentsys-dev dev-install opencode`. So the original pitfall - two copy paths that drift if only one is updated - still holds; only the second path's identity changed. The checklist's "Files to Update" table gets its second row back pointing at scripts/dev-install.js, and the pitfall, the adapters contributing checklist, the replaced test block's comment, and the changelog entry now name both paths.
There was a problem hiding this comment.
This is an auto review done by revuto.
Revuto completed the review and found no evidence-backed concerns.
Problem
adapters/codex/install.shandadapters/opencode/install.shsource every command and skill from$REPO_ROOT/plugins/- a tree that was removed when the plugins moved to standalone repos. Every mapping therefore misses its source file and takes the[WARN] Skippedbranch.set -ecatches nothing, and the script closes with[OK] Installation complete!followed by a list of seven skills it never wrote.They are not inert. Before copying, each script
rm -rfs five skill directories (deslop,review,reality-check-set,pr-merge,review-orchestrator) plus~/.codex/prompts(pr-merge.mdon the OpenCode side). Running one deletes a working install and reports success.The adapter READMEs documented these scripts as the install path, so a user following the docs from a clone would destroy state.
Change
bin/cli.js(agentsys --tool codex|opencode) is the only working install path, and the READMEs now show only that.describe('Install script validation')block from__tests__/opencode-compatibility.test.js. It asserted only on the dead script's text; theXDG_CONFIG_HOMEempty/whitespace semantics it checked are exercised against the live resolver indev-install.test.js.checklists/update-opencode-plugin.mdandmeta/skills/maintain-cross-platform/SKILL.mdatbin/cli.js, and fix the adapter contributing checklist.install.shfromfindOrphanedAdapters' exclusion set inscripts/gen-adapters.js- that scan only ever tests.mdand.jsonnames, so the entry was dead weight.### Removedentry.Verification
npm test3569 passed / 39 skipped, 88 suites.npm run validate,node scripts/gen-adapters.js --check,npm run gen-docs:checkall clean.Review history
The first revision of this PR made the scripts executable and added a note calling them a deprecated fallback that "still works from a clone". That was wrong on the facts, and
chmod +xcut against the goal by lowering the barrier to running a script that can only destroy state. Caught in review; this revision takes the deletion route instead.