fix(amp): restore root plugin entry in install script - #488
Open
IceCodeNew wants to merge 2 commits into
Open
Conversation
The hardened install.sh from 0c7ee11 added atomic staging/rollback but dropped the Amp-discoverable root plugin entry (plugins/nowledge-mem.ts). Amp only loads single-file plugins from a root .ts/.js entry inside the plugins directory; a bare bundle dir is not discoverable, so `amp plugins list` showed nothing after install. The README and the zh integration docs both describe the root entry, so the script regressed. Restore the entry inside the existing atomic flow: - stage plugins/nowledge-mem.ts alongside the bundle and skill - add it to restore_previous and install_staged so a failed step rolls back entry + bundle + skill together (no partial update state) - add pre-flight checks for the staged entry - re-run is a safe atomic update of all three Tests: - static contract test re-asserts PLUGIN_ENTRY_DEST/STAGED_ENTRY/BACKUP_ENTRY and the re-export line that 0c7ee11 removed - new: first install creates entry+bundle+skill, entry target resolves - new: re-run is idempotent (artifacts stable) - new: pre-flight failure leaves a prior install untouched README troubleshooting now points at the root entry file. The zh integration doc already matches; no change there.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe Amp installer now creates a root ChangesAmp plugin entry installation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Installer
participant StagedSource
participant AmpPlugins
Installer->>StagedSource: Stage entry, bundle, and skill
Installer->>StagedSource: Validate entry export and artifacts
Installer->>AmpPlugins: Back up existing installation
Installer->>AmpPlugins: Install staged artifacts
Installer->>AmpPlugins: Restore backups on failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
IceCodeNew
marked this pull request as ready for review
August 9, 2026 12:37
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@nowledge-mem-amp-plugin/README.md`:
- Line 136: Update the “Plugin not loading” troubleshooting instruction to use
the valid installer invocation from the community repository root: bash
nowledge-mem-amp-plugin/scripts/install.sh. Alternatively, explicitly instruct
users to change into nowledge-mem-amp-plugin before running
./scripts/install.sh.
In `@nowledge-mem-amp-plugin/scripts/install.sh`:
- Around line 46-53: The restore_previous function must not remove live
artifacts unless their backups were successfully created and replacements
occurred. Track backup and replacement success independently for the plugin
bundle, skill, and entry artifacts; conditionally remove only replaced
destinations and restore only valid backups. Add regression coverage that forces
each backup mv operation to fail and verifies the original artifacts remain
intact.
🪄 Autofix
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 Plus
Run ID: 679cf0b7-524d-49db-8a36-4cfa127e6ea1
📒 Files selected for processing (3)
nowledge-mem-amp-plugin/README.mdnowledge-mem-amp-plugin/scripts/install.shtests/plugin_e2e/test_key_plugins_e2e.py
CodeRabbit review of PR nowledge-co#488. restore_previous removed PLUGIN_DEST/SKILL_DEST/PLUGIN_ENTRY_DEST unconditionally, then restored only backups that existed. If a backup mv failed (e.g. bundle backup at the entry-then-bundle step), the live artifact was still in place (failed mv keeps its source), but restore_previous deleted it and had no backup to restore from — the live bundle/skill was destroyed. Track each successful backup with a flag (ENTRY/BUNDLE/SKILL_BACKED_UP, set to 1 only after the mv succeeds). restore_previous now removes only destinations whose backup succeeded and restores only those backups, so the rm and restore sets always match regardless of where the sequence fails. Also fix README troubleshooting: ./scripts/install.sh is not valid from the repo root; use bash nowledge-mem-amp-plugin/scripts/install.sh (matches Setup and integrations.json). Tests: - new test_amp_install_script_preserves_prior_install_when_backup_move_fails: chmod 0500 the live bundle dir so the bundle backup mv fails; assert prior entry+bundle+skill all intact afterwards - existing amp tests still green Verified: vitest 189 passed, shellcheck 0 findings, pytest -k amp 6 passed.
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.
Problem
The install script hardened in 0c7ee11 added atomic staging/rollback but silently dropped the Amp-discoverable root plugin entry (
plugins/nowledge-mem.ts). Amp only loads single-file plugins from a root.ts/.jsentry inside the plugins directory (a rootfoo.tsmasks afoo/directory — see Amp Owner's Manual). With only the bundle dir installed,amp plugins listshows nothing and the plugin never loads.The README (line 35) and the zh integration doc both correctly describe the root entry — the script regressed, not the docs. The same commit also removed the static-contract assertions for the entry, which is how it slipped past CI.
Root cause
Commit
0c7ee116("fix(amp): harden bootstrap and capture follow-up") rewroteinstall.shwithSTAGING_DIR/BACKUP_*/restore_previous/install_staged, but the rewrite only copied the bundle dir and omitted the root entry that the prior merge had installed.Fix
Restore the root entry inside the existing atomic flow (keeps all the proven rollback safety):
nowledge-mem-amp-plugin/scripts/install.shPLUGIN_ENTRY_DEST, stageentry.tswithexport { default } from "./nowledge-mem/src/index.ts".restore_previousandinstall_stagedto back up / move / restore the entry alongside the bundle and skill — any step failing restores all three (no partial update state).XDG_CONFIG_HOMEsupport unchanged.integrations.json).Out of scope
Summary by CodeRabbit
New Features
Bug Fixes
Documentation