fix(plugin-store): reject an empty SDK tree without failing the healthy shape (BLO-31857)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Those agents' capabilities come from plugins, each activated by plugin-loader spawning a worker that imports @paperclipai/plugin-sdk
• Before spawning, activatePlugin runs checkSharedDependencyConsistency and throws on !consistent, to catch a torn store fast instead of hanging 60s on the worker initialize handshake (BLO-18384)
• That guard treated absence on either side as "no claim to disagree with", so an install dir with nothing recorded and nothing installed came back congruent — a fixture that set up no store at all passed activation while asserting nothing
• The naive tightening (require both metadata sources present) is an outage: (lock absent)/(installed ok) is the measured healthy steady state of every isolated install dir in production, so it would fail three plugins at once
• This pull request rejects only (absent)/(absent), as a distinct problem: "not_installed", and pins the healthy shape as congruent with named regression tests
• The benefit is that the guard can no longer be satisfied vacuously, while the shape production actually runs is protected by tests that say so out loud

Linked Issues or Issue Description

• Refs BLO-31857 (carried forward from BLO-28656 AC3 second clause + AC4)
• Refs BLO-18384 — original torn-store guard this hardens
• Refs BLO-20961 — the isolated-install mechanism whose healthy shape must not regress

No duplicate or related GitHub PRs found. Searched open + closed PRs for not_installed,
checkSharedDependencyConsistency, plugin-sdk, 31857, 28656, consistency:
only unrelated hits (#1641 SDK idempotent comments, #1585 CI review guard).

What Changed

• checkSharedDependencyConsistency now fails closed when the SDK is neither recorded in package-lock.json nor installed under node_modules, reported as a new, distinct problem: "not_installed".
• Both single-sided absences stay congruent, deliberately and for different reasons, now documented in the function's doc comment:
  • (lock absent)/(installed ok) — the normal peer-dependency-only shape; measured on the live PVC 2026-09-04 for @lucitra/paperclip-plugin-secrets, @lucitra/paperclip-plugin-chat and @penstock/paperclip-plugin.
  • (lock ok)/(installed absent) — already owned by the boot-time SDK_INSTALL_RACE_RETRY_DELAYS_MS retry.
• New SDK_NOT_INSTALLED_ERROR_MARKER for the refusal message, intentionally not reusing TORN_STORE_ERROR_MARKER: reconcileLegacyIsolatedInstallsAtStartup un-latches rows carrying that marker, which would revive an empty tree into the same failure on the next boot.
• New test block BLO-31857: the healthy isolated-tree shape must stay congruent (outage guard) covering the peer-dependency-only shape (lucitra.plugin-secrets), the operator-installed / non-bundled row (paperclip-plugin-hindsight, asserted absent from BUNDLED_PLUGIN_PACKAGES), a sweep over every ISOLATED_SDK_PLUGIN_PACKAGES entry, the recheck fast path, and transient-boot settling.
• New end-to-end activation tests: a peer-dependency-only isolated plugin activates, and an empty install dir refuses to spawn a worker with a message distinct from the torn-store one.
• plugin-database.test.ts: refreshes persisted manifests from disk before activation was the vacuous fixture this change exposes — it drove loadSingle against the real ~/.paperclip/plugins. Given an isolated store with the SDK installed, so it tests manifest refresh rather than ambient disk state.
• Comment at the guard recording why no resolvability probe is used, so one is not added later without re-measuring.

Verification

cd server && npx vitest run src/__tests__/plugin-store-consistency.test.ts → 25 passed.

Full activation-path sweep (every test file that calls loadSingle/loadAll or instantiates pluginLoader) → 74 passed / 7 files:

npx vitest run \
  src/__tests__/plugin-store-consistency.test.ts \
  src/__tests__/plugin-isolated-store-migration.test.ts \
  src/__tests__/plugin-lifecycle-restart.test.ts \
  src/__tests__/plugin-database.test.ts \
  src/__tests__/ccrotate-plugin-retirement.test.ts \
  src/__tests__/plugin-activation-retry.test.ts \
  src/__tests__/plugin-activation-retry-provenance.test.ts

plus src/__tests__/plugin-install-autobuild.test.ts → 9 passed.
npx tsc -p server/tsconfig.json --noEmit → clean (one pre-existing unrelated
packages/adapter-utils acpx/runtime error, untouched by this PR).

The fixture discriminates. A temporary harness importing only symbols that exist on
master head was run against master's plugin-loader.ts and then against this branch's.
(lock absent)/(installed ok) passes both times; (absent)/(absent) fails on master and
passes here:

(lock absent)/(installed ok) — identical before and after:

{ "packageName": "@paperclipai/plugin-sdk", "lockfileVersion": null,
  "installedVersion": "2026.817.0", "lockfileState": "missing",
  "installedState": "ok", "consistent": true, "problem": null, "diagnostic": null }

(absent)/(absent) on master 595e2beb — the vacuous pass:

{ "packageName": "@paperclipai/plugin-sdk", "lockfileVersion": null,
  "installedVersion": null, "lockfileState": "missing",
  "installedState": "missing", "consistent": true, "problem": null, "diagnostic": null }

(absent)/(absent) on this branch:

{ "packageName": "@paperclipai/plugin-sdk", "lockfileVersion": null,
  "installedVersion": null, "lockfileState": "missing",
  "installedState": "missing", "consistent": false,
  "problem": "not_installed", "diagnostic": null }

Independent evidence that the vacuous pass was real, not theoretical: with the guard
tightened, plugin-database.test.ts failed because it had been activating a plugin with
no SDK anywhere on disk. That is the defect, caught by the change.

Production baseline for the post-deploy check: paperclip_plugin_error is 0 for all 11
plugins, including lucitra.plugin-secrets, penstock.paperclip-plugin,
paperclip-plugin-hindsight and paperclip-chat.

Risks

Low, but the risk is concentrated in one place and worth stating precisely.

• The shape this could break, and why it does not. Rejecting (absent)/(absent) fails activation for any plugin whose install dir has no SDK. That cannot be a healthy production shape: an isolated dir's parent chain deliberately excludes the shared store (the whole point of BLO-20961), so a plugin there with no local SDK could not resolve it and would already be erroring — and all four isolated plugins are at paperclip_plugin_error = 0. For shared-store plugins the SDK sits in the store the guard reads.
• Boot race. (absent)/(absent) is genuinely transient while the concurrent boot npm install runs. It is safe because every inconsistent result routes through checkSharedDependencyConsistencyAfterRecheck, which returns as soon as the SDK lands (first recheck at 500ms) and only fails closed on a mismatch stable past 10s. A test covers exactly this. The cost of being wrong is bounded and visible: a plugin marked error with an actionable message, not a silent 60s hang.
• Not verified from inside the cluster. The install-dir shapes are from the BLO-31857 live measurement of 2026-09-04, not re-stated this run — this run had k8s-ro only, with no exec verb. The argument above is mechanism-based plus the error-metric baseline, and the deploy check below is what would catch it.
• Recovery path deliberately narrowed. A row failing not_installed is not un-latched by reconcileLegacyIsolatedInstallsAtStartup, by design: relocation does not populate an empty tree, so reviving it would just re-fail. It needs a real install.
• Post-deploy check: paperclip_plugin_error must stay 0 for lucitra.plugin-secrets, lucitra.plugin-chat / paperclip-chat, penstock.paperclip-plugin and paperclip-plugin-hindsight for 24h.

Model Used

• Claude Opus 4.5 (claude-opus-4-5, 1M context) via Claude Code, extended thinking, with tool use (filesystem/bash/git/gh, Kubernetes read-only, Prometheus).

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, no UI surface
• I have updated relevant documentation to reflect my changes
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending first CI run
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending review
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-31857
🔗 Paperclip issue: BLO-18384
🔗 Paperclip issue: BLO-28656
🔗 Paperclip issue: BLO-20961

[allyblockcast]

@ally please review at head 41ca614.

Focus, in priority order:

1. Is (absent)/(absent) genuinely safe to reject? This is the only
   behavioural change and the only outage surface. It fails activation for any
   plugin whose install dir has no SDK. My argument that no healthy production
   plugin is in that shape is in the PR Risks section and is mechanism-based,
   not re-measured from inside the cluster this run (k8s-ro only, no exec).
   If you can construct a healthy shape I have missed — a plugin resolving the
   SDK from an ancestor of its install dir, say — that is the finding that
   matters most.
2. Boot race. I claim a transient (absent)/(absent) is safe because
   every inconsistent result routes through
   checkSharedDependencyConsistencyAfterRecheck. Please check I have not
   introduced a path that reaches the throw without the recheck.
3. Marker choice. The refusal deliberately avoids
   TORN_STORE_ERROR_MARKER so
   reconcileLegacyIsolatedInstallsAtStartup does not un-latch an empty
   tree. Is narrowing that recovery path right, or should an empty tree be
   revived and reinstalled instead?
4. I deliberately did not reject (lock ok)/(installed absent), though
   BLO-31857's own table says to, because an existing deliberate test pins it
   congruent and the boot install race already owns it. Sanity-check that call.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 41ca614

The core judgement is right, and the part that is easiest to get wrong here is done well: (lock absent)/(installed ok) stays congruent, and it is pinned by seven tests plus an end-to-end activation test, so the outage this narrowly avoids cannot be reintroduced silently. The two findings below are both about the blast radius of the new not_installed verdict rather than about the verdict itself.

Critical Issues (0)

Important Issues (2)

• [gstack/review + native-codex] server/src/services/plugin-loader.ts:472 — the new notInstalled verdict is unscoped, so it also fires for every non-isolated plugin, whose consistency check runs against the shared store rather than a dir the server installed into.

  • plugin-loader.ts:2866 resolves pluginInstallDir = activePlugin.installDir ?? localPluginDir, and local-filesystem/cwd/node_modules discovery deliberately persists installDir = null (:2336, :2472). For a plugin loaded from the repo's own node_modules, the SDK resolves by walking up to the workspace root — ~/.paperclip/plugins is never consulted at import time and may legitimately be (absent)/(absent). That tree previously activated fine; it now fails closed after the full ~28.5s recheck.
  • The PR's own diff is the evidence that this path was live and working: plugin-database.test.ts needed the new createSdkPopulatedPluginStore() fixture because activation had been succeeding with a completely empty store. Calling that fixture vacuous is fair — but the same shape in a dev checkout is a working setup, not a vacuous one, and it is not covered by either the measured production shape or the new tests.
  • Recommendation: scope the verdict to install dirs the server owns — gate notInstalled (or the throw at :2900) on activePlugin.installDir !== null / isIsolatedSdkPluginPackage(packageName). That keeps the guard exactly where BLO-31857 was measured (isolated dirs the server npm-installs) and still rejects the empty-isolated-tree case, without failing a tree that resolves the SDK by walking up.

• [pr-review-toolkit/errors] server/src/services/plugin-loader.ts:557 — decoupling from TORN_STORE_ERROR_MARKER is correctly reasoned, but it leaves not_installed with no automatic recovery path at all, and the docstring does not say so.

  • reconcileLegacyIsolatedInstallsAtStartup skips any errored row that does not carry the torn marker (:1899), and even for torn rows only clears status when installDir actually moved (:1903). registry.updateStatus(... "ready" ...) at :1906 is the only error→ready transition in the file, so a row latched by the new marker stays errored until an operator intervenes.
  • That makes the 28.5s recheck the single line of defence for a transient. The sibling shape (lock ok)/(installed absent) gets a second independent budget — the SDK_INSTALL_RACE_RETRY_DELAYS_MS worker-spawn retry at :3014 — but (absent)/(absent) now throws before a worker is ever spawned, so it never reaches that loop. The shape with strictly less information gets strictly less resilience, and a boot npm install slower than ~28.5s (cold cache, slow PVC) permanently disables the plugin. The new test asserting the transient settles only proves the fast case.
  • Recommendation: add SDK_NOT_INSTALLED_ERROR_MARKER to the startup un-latch set with a re-probe rather than a relocation test — clear to ready iff checkSharedDependencyConsistency now returns consistent, independent of whether installDir changed. That preserves the intent ("an empty tree needs an actual install") while letting the row recover once the install lands, instead of requiring a human.

Suggestions (3)

• [pr-review-toolkit/tests] server/src/__tests__/plugin-store-consistency.test.ts:975 — expect(elapsedMs).toBeGreaterThanOrEqual(10_000) / toBeLessThan(35_000) (:976) hard-codes bounds derived from SDK_STORE_CONSISTENCY_MIN_STABLE_MISMATCH_MS and the delay ladder (500+1500+3500+7500+15500, so the recheck returns at ~13s). Deriving them from the exported constants would keep the assertion honest if the ladder is retuned — and it is worth noting this one case adds ~13s of wall clock to the suite.
• [pr-review-toolkit/tests] server/src/__tests__/plugin-store-consistency.test.ts:351 — writeLockfileOmitting reads the file back to assert on a JSON literal it wrote three lines earlier. The fixture guard is a good instinct, but it can only fail if someone edits that same literal; asserting on the object (or a comment) avoids the I/O round-trip.
• [native-codex] server/src/__tests__/plugin-store-consistency.test.ts:904 — ISOLATED_SDK_PLUGIN_PACKAGES[0] (also :925) ties the new activation test to list ordering. The sweep test already covers every entry, so naming the intended package explicitly would say what the test means and survive a reordering of the list.

Strengths

• The asymmetry between the two single-sided absences is documented with the measurement that justifies it (live PVC, 2026-09-04, all three isolated dirs), plus a pointer to the unmerged working tree that got it wrong. That is the rare comment that will actually stop the next person from "tightening" this.
• Recording that require.resolve and import.meta.resolve were both tried and rejected against the real layout — with the reason (ESM-only exports, no require condition) — pre-empts the obvious stronger gate and explains why it cannot be used. Negative results are usually the part that gets lost.
• problem is widened as a discriminated union member rather than folded into an existing value, and there is a test asserting all three values are distinct, so a future collapse fails loudly.
• The discriminating test ("still rejects that same tree once the installed SDK is removed") is the right control: identical fixture, one variable, opposite verdict. It shows the acceptance is not blanket permissiveness.
• paperclip-plugin-hindsight is covered explicitly with an assertion that it is isolated-but-not-bundled — verified accurate against both lists at this head. A fixture built only from bundled rows would have missed that row.

Recommended Action

1. No Critical issues — nothing blocks on correctness of the congruent path.
2. Scope the not_installed verdict to server-owned install dirs (Important #1) before merge; it is the one change that can break a working dev checkout.
3. Give not_installed a re-probe-based un-latch (Important #2) so a slow boot install self-heals rather than requiring an operator.
4. Take the Suggestions opportunistically.