Repository navigation
refactor(plugins): remove the version 1 provider plugin contract (#3759) - #4504
Conversation
Drop the v1 loader adapter, the manifest-driven discovery and store branches, the manifest credential and connection fallbacks, and the install-time provider collision checks. A store package is discovered through its exports only, and everything it declares comes from the object its factory returns. The registry settles a duplicate provider id by first claim. Remove the v1 entries and manifest declarations from TestMu AI and Revyl, and document the migration to an AgentDevicePluginFactory in ./agent-device.
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 40 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/release-workspace.mjs">
<violation number="1" location="scripts/release-workspace.mjs:74">
P2: This predicate only matches packages that export `./agent-device`, but the host's discovery also accepts plugins that export the factory from `.` or `main`. Those packages can still ship `agent-device` as a runtime dependency and pass this check. Consider also treating a package whose root export is a plugin factory as a plugin, or documenting that the root-fallback form is unsupported for publishing.</violation>
</file>
<file name="packages/plugins/src/registry.ts">
<violation number="1" location="packages/plugins/src/registry.ts:34">
P3: This doc comment suggests the id is claimed before loading, but for non-dormant candidates the claim happens after `load()` runs, and a load failure still claims it. Reword it to say the id is claimed whether or not the plugin loads, and that dormant candidates claim it without loading.</violation>
</file>
<file name="docs/adr/0034-provider-plugin-registry.md">
<violation number="1" location="docs/adr/0034-provider-plugin-registry.md:26">
P2: This ADR bullet says discovery uses only `exports` and reads nothing from `package.json` to choose an entry, but `discoverPluginEntry` still falls back to `package.json` `main` when `exports` is absent. Either remove that `main` fallback in `discoverPluginEntry` so the code matches the documented exports-only contract, or document the `main` fallback here.</violation>
</file>
<file name="packages/plugins/src/store.ts">
<violation number="1" location="packages/plugins/src/store.ts:245">
P3: This doc comment says install accepts a package when its entry loads as a plugin, but `changePlugin` never imports the entry. Install only checks package identity, version, and that the entry resolves to a readable file; the default-export check runs later in `loadInstallation`. Reword the comment to match.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| if (packed.agentDevicePlugin) unpublishableNames.add(rootPackage.manifest.name); | ||
| // A provider plugin exports its factory as `./agent-device` and imports only types from | ||
| // agent-device, so it must never carry the core as a runtime dependency. | ||
| if (packed.exports && Object.hasOwn(packed.exports, './agent-device')) |
There was a problem hiding this comment.
P2: This predicate only matches packages that export ./agent-device, but the host's discovery also accepts plugins that export the factory from . or main. Those packages can still ship agent-device as a runtime dependency and pass this check. Consider also treating a package whose root export is a plugin factory as a plugin, or documenting that the root-fallback form is unsupported for publishing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At scripts/release-workspace.mjs, line 74:
<comment>This predicate only matches packages that export `./agent-device`, but the host's discovery also accepts plugins that export the factory from `.` or `main`. Those packages can still ship `agent-device` as a runtime dependency and pass this check. Consider also treating a package whose root export is a plugin factory as a plugin, or documenting that the root-fallback form is unsupported for publishing.</comment>
<file context>
@@ -69,7 +69,10 @@ function checkPublishedDependencies(packed) {
- if (packed.agentDevicePlugin) unpublishableNames.add(rootPackage.manifest.name);
+ // A provider plugin exports its factory as `./agent-device` and imports only types from
+ // agent-device, so it must never carry the core as a runtime dependency.
+ if (packed.exports && Object.hasOwn(packed.exports, './agent-device'))
+ unpublishableNames.add(rootPackage.manifest.name);
for (const field of ['dependencies', 'optionalDependencies', 'peerDependencies']) {
</file context>
| their manifest before any code runs and load through an adapter until 0.22 removes them (#3759). | ||
| A package that keeps the manifest and also exports `./agent-device` loads as v2; hosts without | ||
| v2 still read the manifest. | ||
| - A store package is discovered only through its exports: `./agent-device`, or the package root when |
There was a problem hiding this comment.
P2: This ADR bullet says discovery uses only exports and reads nothing from package.json to choose an entry, but discoverPluginEntry still falls back to package.json main when exports is absent. Either remove that main fallback in discoverPluginEntry so the code matches the documented exports-only contract, or document the main fallback here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/adr/0034-provider-plugin-registry.md, line 26:
<comment>This ADR bullet says discovery uses only `exports` and reads nothing from `package.json` to choose an entry, but `discoverPluginEntry` still falls back to `package.json` `main` when `exports` is absent. Either remove that `main` fallback in `discoverPluginEntry` so the code matches the documented exports-only contract, or document the `main` fallback here.</comment>
<file context>
@@ -23,10 +23,9 @@ Status: accepted. Umbrella #3747; contract and registry #3755.
- their manifest before any code runs and load through an adapter until 0.22 removes them (#3759).
- A package that keeps the manifest and also exports `./agent-device` loads as v2; hosts without
- v2 still read the manifest.
+- A store package is discovered only through its exports: `./agent-device`, or the package root when
+ that subpath is absent. Nothing is read from `package.json` to choose an entry or to declare a
+ provider, connection capabilities, or credential variables (#3759).
</file context>
| package?: string; | ||
| version?: string; | ||
| /** Known before the plugin loads, such as a v1 manifest; a load failure still claims it. */ | ||
| /** Claims `provider` without loading it, so a later candidate with that id is a duplicate. */ |
There was a problem hiding this comment.
P3: This doc comment suggests the id is claimed before loading, but for non-dormant candidates the claim happens after load() runs, and a load failure still claims it. Reword it to say the id is claimed whether or not the plugin loads, and that dormant candidates claim it without loading.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/plugins/src/registry.ts, line 34:
<comment>This doc comment suggests the id is claimed before loading, but for non-dormant candidates the claim happens after `load()` runs, and a load failure still claims it. Reword it to say the id is claimed whether or not the plugin loads, and that dormant candidates claim it without loading.</comment>
<file context>
@@ -31,11 +31,11 @@ export type PluginCandidate = Readonly<{
package?: string;
version?: string;
- /** Known before the plugin loads, such as a v1 manifest; a load failure still claims it. */
+ /** Claims `provider` without loading it, so a later candidate with that id is a duplicate. */
provider?: string;
/** Claims `provider` without registering or listing: a built-in that is not configured. */
</file context>
| /** Claims `provider` without loading it, so a later candidate with that id is a duplicate. */ | |
| /** Known before the plugin loads, such as a dormant built-in; a load failure still claims it. */ |
| * Installs, updates or removes one plugin selection. A package is accepted when its entry loads as | ||
| * a plugin; a provider-id clash with another plugin or a built-in is the registry's to settle when | ||
| * the daemon next composes its plugins. |
There was a problem hiding this comment.
P3: This doc comment says install accepts a package when its entry loads as a plugin, but changePlugin never imports the entry. Install only checks package identity, version, and that the entry resolves to a readable file; the default-export check runs later in loadInstallation. Reword the comment to match.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/plugins/src/store.ts, line 245:
<comment>This doc comment says install accepts a package when its entry loads as a plugin, but `changePlugin` never imports the entry. Install only checks package identity, version, and that the entry resolves to a readable file; the default-export check runs later in `loadInstallation`. Reword the comment to match.</comment>
<file context>
@@ -207,11 +241,15 @@ async function stagePlugin(
}
+/**
+ * Installs, updates or removes one plugin selection. A package is accepted when its entry loads as
+ * a plugin; a provider-id clash with another plugin or a built-in is the registry's to settle when
+ * the daemon next composes its plugins.
</file context>
| * Installs, updates or removes one plugin selection. A package is accepted when its entry loads as | |
| * a plugin; a provider-id clash with another plugin or a built-in is the registry's to settle when | |
| * the daemon next composes its plugins. | |
| * Installs, updates or removes one plugin selection. A package is accepted when its entry resolves | |
| * to a readable file inside the package; its default export is checked when the daemon loads it. | |
| * A provider-id clash is the registry's to settle when the daemon composes its plugins. |
…removal discoverPluginEntry is the only caller of resolvePluginEntry, and the store's version-selector check lost its second caller with the manifest branches, so both become module-local. Drop the deleted v1 entries from the Fallow entry list. Removes the dead-export findings the fallow audit raised at head.
Apex Sentinel · ✅ ApproveSince the last review: 0 new · 0 still open · 1 no longer raised No open findings. 1 finding below 💬 Talking to SentinelIn a finding's thread, reply with:
On this conversation, comment Earlier reviewsReview of
|
| Where | Finding |
|---|---|
scripts/release-workspace.mjs:72 |
Publish guard keys on raw package.json exports, which publishConfig can hide |
Reviewed with this repository's own Sentinel configuration (ed73e7912e56).
![apex-sentinel-staging[bot]](https://avatars.githubusercontent.com/in/5196457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Apex Sentinel: 1 new finding on 246cac4. The verdict and the full list are in the summary comment.
| // A provider plugin exports its factory as `./agent-device` and imports only types from | ||
| // agent-device, so it must never carry the core as a runtime dependency. | ||
| if (packed.exports && Object.hasOwn(packed.exports, './agent-device')) | ||
| unpublishableNames.add(rootPackage.manifest.name); |
There was a problem hiding this comment.
No longer raised on f273967.
🟡 Minor · Publish guard keys on raw package.json exports, which publishConfig can hide
packed.exports is read from the source package.json, but provider packages declare their export map under publishConfig.exports. Object.hasOwn(packed.exports, './agent-device') is therefore false for those packages, so a real dependency on private root agent-device packs and publishes unpublishably. The previous trigger, packed.agentDevicePlugin, was a plain top-level field and did not have this dependency.
Details
Also raised here
Also raised at this location by persona senior-developer: Provider publish guard misses a re-mapped ./agent-device export — checkPublishedDependencies adds agent-device to the unpublishable set only when Object.hasOwn(packed.exports, './agent-device'), so a provider package whose published export key differs by whitespace or nesting (for example "./agent-device " or exports wrapped as an array) installs its factory and ships agent-device as a runtime dependency without tripping the guard the removed packed.agentDevicePlugin check used to raise unconditionally for plugin packages. Consider keying the guard on the provider-package identity (the packages/provider-*/src/index.ts set, or the presence of a default export in a dist/agent-device.* file) rather than on an exact export key match.
Also raised at this location by persona javascript: Publish guard reads only top-level exports, so a publishConfig-only plugin bypasses it — node scripts/release-workspace.mjs pack packs @agent-device/testmu or @agent-device/revyl with a package.json that declares ./agent-device only under publishConfig.exports, while the top-level exports map has no such key: the new own-property test sees nothing, agent-device (the private root) is not added to the unpublishable set, and a tarball shipping the workspace core as a runtime dependency packs clean. The old guard keyed on agentDevicePlugin, a field pnpm pack copies verbatim, so it could not be hidden from. The check should also consider the publishConfig.exports map that npm actually publishes.
persona: senior-developer, maintainability, javascript
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if it is wrong — I will re-check the cited code.
Point the traversal fixture at the file that exists outside the package, say the entry contract as it is (root export, which is main without exports), move the v1 manifest's connection to connection.capabilities in the migration note and separate the incompatible_plugin and invalid_plugin cases, claim the built-in id with a non-routing provider in the store test, and state what install checks.
|
No code blockers found at f273967. CI has 20 successful checks and 11 skipped checks. There are no conflicts. The release guard and packed-manifest threads do not apply: all four public plugin packages export Not blocking: the registry wording and other v1 references in comments and test names could be cleaned up, two loader tests overlap, and packed TestMu/Revyl smoke runs would strengthen the package evidence. |
thymikee
added
the
ready-for-human
…) (#4504) * refactor(plugins): remove the version 1 provider plugin contract (#3759) Drop the v1 loader adapter, the manifest-driven discovery and store branches, the manifest credential and connection fallbacks, and the install-time provider collision checks. A store package is discovered through its exports only, and everything it declares comes from the object its factory returns. The registry settles a duplicate provider id by first claim. Remove the v1 entries and manifest declarations from TestMu AI and Revyl, and document the migration to an AgentDevicePluginFactory in ./agent-device. * refactor(plugins): keep entry resolution module-private after the v1 removal discoverPluginEntry is the only caller of resolvePluginEntry, and the store's version-selector check lost its second caller with the manifest branches, so both become module-local. Drop the deleted v1 entries from the Fallow entry list. Removes the dead-export findings the fallow audit raised at head. * refactor(plugins): answer the review findings on the v1 removal Point the traversal fixture at the file that exists outside the package, say the entry contract as it is (root export, which is main without exports), move the v1 manifest's connection to connection.capabilities in the migration note and separate the incompatible_plugin and invalid_plugin cases, claim the built-in id with a non-routing provider in the store test, and state what install checks.
Summary
Part of #3759 (section 1). Removes the version 1 provider plugin contract from 0.22. 40 files: +679/−1072.
./agent-deviceexport, else the root export; entry containment is kept.agentDevicePluginmanifest from@agent-device/testmuand@agent-device/revyl; the publish guard now keys on the./agent-deviceexport.plugins.mdcarries the migration note; ADR 0034 states the current truth.Validation
At head
f27396742f(base2b2eb83131, origin/0.22):pnpm install --frozen-lockfile,pnpm build,pnpm format,pnpm check:quick,pnpm check:doc-prose: pass.vitest run --project unit-coreover plugins, test-kit, provider packages, provider policy, connect, daemon composition, ADR guard: 637 tests pass.fallow audit --base 2b2eb83131: pass; its one clone group is inherited (pre-existing proxy/tsdown duplication).pnpm check:affected --run --full --base origin/0.22 --head f27396742f: passed,exactCommit: true, clean checkout; 41 checks passed, 25 skipped as GitHub-authoritative or parkedworkflow_dispatchlanes. Selection was fail-open (full set), not narrowed.Limits
No v1 shims, compatibility fallbacks, or provider-name hacks. The v2 factory and
./agent-devicecontract, fixed plugin set, first-claim and error isolation are unchanged. No managed-client, ambient-SDK, BrowserStack or Maestro work here.