fix(plugins): isolate third-party SDK-consuming plugins from the shared store (BLO-20961) - #1138
Merged
allyblockcast[bot] merged 3 commits intoAug 15, 2026
Conversation
Author
1 similar comment
Author
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
Author
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: ef139fa
Critical Issues (0)
Important Issues (1)
- [code]
server/src/services/plugin-loader.ts:2556— Existing installations of the affected plugins remain on the torn shared store after this migration.loadSingleresolvespluginInstallDirexclusively fromactivePlugin.installDir ?? localPluginDir; the additive migration leavesinstallDirnull, so all existing rows continue to load and fail the consistency check againstlocalPluginDir. Isolation is only selected during a later install/upgrade (:2136), with no migration, startup relocation, or explicit reconciliation path for the two known broken rows.- Move/reinstall the allowlisted existing plugin records into their resolved isolated directories as part of rollout, or add a safe startup/upgrade migration that persists the isolated directory before activation. Cover the existing-null-row path in an integration test.
Suggestions (0)
Strengths
- The new install-directory persistence keeps later upgrade and cleanup operations targeted at the plugin's actual filesystem tree.
- The two-boot consistency test directly models the shared-store tear rather than only testing a successful first install.
Recommended Action
- Address the Important issue before merge so the affected production plugins recover on the deployment that introduces this change.
allyblockcast
Bot
force-pushed
the
platformsre/blo-20961-plugin-sdk-installdir-isolation
branch
from
August 14, 2026 10:38
ef139fa to
00baaa0
Compare
…ed store (BLO-20961) `copyWorkspaceSdkFiles()` unconditionally re-vendors the workspace plugin-sdk fork into the shared plugins node_modules on every boot, independently of npm reconciliation. Any plugin sharing that store (lucitra.plugin-secrets, paperclip-plugin-hindsight) has its installed @paperclipai/plugin-sdk torn from package-lock.json every restart (BLO-18384/BLO-18405), which the existing checkSharedDependencyConsistency guard (PR #804) detects but does not prevent. Give third-party plugins that declare a real @paperclipai/plugin-sdk dependency their own install directory (plugins-isolated/<pkg>), outside the path copyWorkspaceSdkFiles() and the fork-copy touch, so they are structurally immune rather than merely reconciled-until-next-boot. Isolation is automatic based on package name (ISOLATED_SDK_PLUGIN_PACKAGES) via installPlugin's default installDir resolution — no route change needed. Persist the resolved installDir on the plugin row so upgrade and uninstall target the same directory the plugin actually lives in. paperclip-chat and penstock.paperclip-plugin are included in the isolation list per the mechanism (any third-party SDK consumer generalizes), but out of scope for this issue's two-boot verification, which covers only lucitra.plugin-secrets / paperclip-plugin-hindsight per the CEO's 2026-08-05 scope decision. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…(BLO-20961) The BLO-20961 isolation change makes `installDir` a required (nullable) field on `PluginRecord`. `makePlugin()` in InstanceSidebar.test.tsx builds a full record literal and did not set it, so `tsc -b` over the `ui` workspace failed with TS2741 — which is the sole reason the `Typecheck + Release Registry` lane went red, and `verify` is only its roll-up (it reports `Upstream lane(s) reported failure: typecheck_release_registry`), not an independent failure. Defaults to `null` (= the shared store) to match the sibling nullable fixture fields and the column's own semantics.
allyblockcast
Bot
force-pushed
the
platformsre/blo-20961-plugin-sdk-installdir-isolation
branch
from
August 14, 2026 10:41
00baaa0 to
779d551
Compare
13 tasks
…d store (BLO-20961) Ally's Important finding on this PR: the installDir column is additive, so every pre-existing row carries installDir = NULL and keeps resolving to the shared plugin store. That includes the two plugins this issue exists to fix. On the worker tier the shared store is torn BY CONSTRUCTION -- index.ts re-vendors the workspace SDK fork (1.0.0) over it on every boot while the lockfile permanently records 2026.513.0 -- so those rows fail the consistency guard 100% of the time and isolation would only ever engage on some later install/upgrade that never comes. AC #1 was not met as the PR stood. Changing activatePlugin's fallback to resolveDefaultInstallDir() is NOT the fix on its own: the isolated dir does not exist yet for these rows, so that only trades a torn-store failure for a missing-entrypoint one. This does the real relocation instead: - reconcileLegacyIsolatedInstall(): npm-install the package into its isolated dir, verify the reinstall is the same plugin identity, then persist installDir -- so later boots resolve somewhere that exists. - reconcileLegacyIsolatedInstallsAtStartup(): run that across installed rows before loadAll() selects by status, and un-latch rows the torn-store guard parked in `error`. loadAll() only selects status='ready', so a row refused once stays invisible to every later boot even after the cause is fixed (five plugins have been latched since 2026-08-01 -- BLO-20410). Only rows whose lastError came from that guard are revived, and only when the relocation actually moved them; best-effort per row so one bad package cannot abort boot. - Wire into loadAll() and loadSingle() (operator re-enable gets the same treatment rather than re-running against the shared store). - Extract TORN_STORE_ERROR_MARKER so the un-latch decision stays coupled to the message that latched the row. Tests in a separate file because node:child_process must be mocked to stub npm install, and promisify(execFile) captures the binding at module init -- hoisting that mock into plugin-store-consistency.test.ts would take the real npm path away from its local-filesystem fixtures. Verified with a control: reverting only the loadAll() wiring fails exactly two of the four new tests with the production symptoms (installDir stays null; status stays error). Existing plugin-store-consistency suite: 15/15.
13 tasks
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
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.
Thinking Path
Linked Issues or Issue Description
Fixes: BLO-20961 (internal Paperclip tracker — no public GitHub issue)
Refs: BLO-18384, BLO-18405 (prior manual repairs of the same recurring tear)
Refs: PR #804 (added the detection-only
checkSharedDependencyConsistencyguard this PR builds on)What Changed
server/src/bootstrap/isolated-sdk-plugins.ts(new):ISOLATED_SDK_PLUGIN_PACKAGESallowlist +resolveDefaultInstallDir()— resolves a package's install dir toplugins-isolated/<sanitized-pkg-name>when it's in the allowlist, else the shared store (unchanged default).server/src/services/plugin-loader.ts:installPlugin/upgradePlugindefaultinstallDirviaresolveDefaultInstallDir()instead of always using the sharedlocalPluginDir.loadSingleresolves the package root, worker entrypoint, and shared-dependency consistency check against the plugin's own persistedinstallDir, not the shared store.cleanupInstallArtifactsnow uninstalls/removes from the plugin's actualinstallDir(previously always targeted the shared store, which would have silently no-op'd and orphaned an isolated plugin's files on uninstall).server/src/services/plugin-registry.ts,packages/db/src/schema/plugins.ts,packages/shared/src/types/plugin.ts,packages/shared/src/validators/plugin.ts,packages/db/src/migrations/0211_plugin_install_dir.sql: new nullableinstallDircolumn onplugins, persisted through install/upgrade so later operations target the right directory.installDir.paperclip-chatandpenstock.paperclip-pluginare included inISOLATED_SDK_PLUGIN_PACKAGES(the mechanism generalizes to any third-party SDK consumer per the CEO's 2026-08-05 scope decision on the issue) but are explicitly out of scope for this PR's verification — onlylucitra.plugin-secrets/paperclip-plugin-hindsightare covered by the two-boot test below.Verification
server/src/__tests__/plugin-store-consistency.test.ts(extended): 3 new tests —resolveDefaultInstallDirisolates every allowlisted package and leaves everything else in the shared store.loadSingletest: an isolated plugin activates successfully even whenlocalPluginDir(the shared store) is torn between "boot 1" and "boot 2".pnpm exec vitest run server/src/__tests__/plugin-store-consistency.test.ts(15/15 pass),plugin-install-autobuild.test.ts(9/9),plugin-lifecycle-restart.test.ts(2/2),plugin-routes-authz.test.ts(49/49) — all green, no regressions from theinstallDirplumbing.pnpm --filter @paperclipai/db check:migrations(numbering + safety) passed.tsc --noEmitoverserver/passed clean.paperclip-0pod restarts post-merge, pasting the version-check output showing continued lockfile/installed congruence, plus aGET /api/plugins/rag-healthsnapshot showing both pluginsreadythroughout — see BLO-20961 acceptance criteria.Risks
plugins.install_dir) — additive, no backfill needed, existing rows default tonull(shared store), matching current behavior.cleanupInstallArtifactsbehavior changes for already-installed isolated plugins: today there are none in the live DB (both target plugins are currently inerrorwithinstallDir = nullpending reconciliation), so this only takes effect on their next install/upgrade after this ships.copyWorkspaceSdkFiles()orautoInstallBundledPlugins()boot ordering — deliberately, since isolation makes the ordering question moot for isolated plugins. The shared store itself can still tear for any plugin not isolated; that's unchanged/expected.Model Used
Claude Sonnet 5 (
claude-sonnet-5[1m]), 1M context window, agentic tool use (Bash, Read/Edit/Write, Task tracking) via Claude Code / Paperclipclaude_k8sadapter. No extended-thinking mode.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template