Repository navigation
feat(plugins): plugin contract v2 and one provider registry - #3767
Conversation
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
All reported issues were addressed across 59 files
You’re at about 92% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
fa82eb7 to
2f64746
Compare
There was a problem hiding this comment.
All reported issues were addressed across 16 files (changes from recent commits).
You’re at about 93% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
2f64746 to
deae993
Compare
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
You’re at about 93% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
deae993 to
447eb9e
Compare
There was a problem hiding this comment.
All reported issues were addressed across 13 reviewed files. 1 file intentionally excluded from review.
You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
447eb9e to
9096044
Compare
|
I reviewed 9096044 and found one defect that must be fixed before merge. CI is green: 23 checks, none failing at this commit. I know of no conflicts.
Not blocking, and you can take or leave these: the built-in providers are described twice, in This PR rewires how the daemon builds every cloud provider runtime, and the only validation shown is check:affected. Could you post one live run at this commit? Start the daemon with Could this land smaller? The diff is 1230 net production lines in one commit. One built-in descriptor list (id, credentials, connection capabilities, lazy setup) could feed both The open cubic-dev-ai thread on the test-only classifier in the harness (#3767 (comment)) is marked resolved but still applies. Please fix the frozen-runtime shutdown path, then post the live Limrun run, before merge. |
9096044 to
105f415
Compare
|
Thanks for the review. Fixed in 105f415 (stack top 7f449c5 passed Blocking: frozen runtime. Live run (this build, fake
Harness classifier: the harness takes pre-built runtimes from each test, so it cannot go through the registry; I rewrote the header so it no longer claims daemon parity. Smaller split? I kept four layers. Contract + registry cannot land without the client-side priming: a v2 plugin declares its connect capabilities in code, so the client has to load the store before the synchronous reads in #3748 / #3722: still blocked on #3773. I'll rebase and add the failed inventory entry once it merges; #3722 would conflict in the same plugin files, and I'd keep this branch's versions and take its R85 rule. |
There was a problem hiding this comment.
1 issue found across 10 files (changes from recent commits).
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
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="packages/daemon/src/server/http/daemon-runtime-composition.test.ts">
<violation number="1" location="packages/daemon/src/server/http/daemon-runtime-composition.test.ts:71">
P2: This test no longer checks that every provider shuts down when one throws. The per-provider isolation moved into `registry.close`, and no test covers it, so a regression to a sequential loop would pass unnoticed. Add a registry test where one registration's `runtime.shutdown()` throws and assert the others still run, or rename this test to match what it now asserts.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
105f415 to
bd57cd7
Compare
|
Thanks for the update. The registry shutdown fix and the per-plugin isolation test are in at bd57cd7, and all 23 checks pass. One piece of evidence is still missing, so I can't call this ready yet. The posted run used a fake LIMRUN_API_KEY. It covered the daemon.json inventory, the credential-fingerprint refusal and a graceful stop. It never served a lease from the Limrun runtime that the registry registers, and it never released that runtime through registry.close. src/provider-device-runtimes.ts:43 now rebuilds every cloud provider runtime through the v2 registry, and daemon shutdown goes through registry.close. A regression in how the registry adapts or stops a real cloud runtime would ship without one exercised lease. The "needs a real account" reason does not hold, because the key is in the main checkout .env. Please load only LIMRUN_API_KEY from that file into the worktree dist, with a fresh AGENT_DEVICE_STATE_DIR. Then run Not blocking, and you can take or leave these: the composition tests in provider-device-runtimes.test.ts still shut down through composition.runtimes, and the daemon tests mock shutdownProviderRuntimes, so nothing exercises composition.shutdown, the daemon's only provider-shutdown route (replacing daemon-composition.ts:129 with The cubic-dev-ai P2 thread on per-plugin shutdown isolation is fixed at this head, because load.test.ts:576-612 now covers it, so you can resolve it: #3767 (comment) I ran no tests and judged the regression tests by reading them. I did not check the output of the fake-key run beyond the comment text, did not read the stacked docs PR #3769, and did not trace the #3748 failed-import inventory entry that you say waits on #3773. No conflicts are known. Before merge, please post the real-key Limrun run at bd57cd7, so the stop goes through registry.close. |
bd57cd7 to
aa98411
Compare
|
Real-key run, as asked. Build: the stack top So a lease was served by the Limrun runtime the registry registers, |
aa98411 to
9467595
Compare
|
Taken in 9467595 (stack top 73a0bbc passed |
|
I found no code problems at 9467595. The earlier findings from bd57cd7 are fixed, and nothing from them remains. The PR now has merge conflicts with main. After #3722 merged, 18 files conflict, including packages/plugins/src/load.ts, manifest.ts and store.ts, src/provider-device-runtimes.ts and provider-policy.ts. Please rebase and re-run the gate. Smoke Tests is still running and shows no failure yet. Every daemon start goes through createDaemonProviderRuntimeComposition and composition.shutdown, which smoke exercises, so a failure there would be relevant. The other 22 checks pass. I ran no tests and judged the new shutdown test by reading it. The real-key Limrun run was on 2dd219b, not on 9467595. I accepted it because the changes since then are test-only plus the .catch rewrite. I did not resolve the conflicts, so the rebased shape of the conflicting files still needs a look. After the rebase and a green gate, this is ready for human review once #3748 lands. |
1fb2e78 to
5ccff7f
Compare
|
Rebased onto main (
Still blocked on #3748 (#3773): not mergeable before it. After it merges I'll rebase and add the failed inventory entry to |
|
Thanks for the rework. In the earlier review (#3767 (comment)) the conflict was the open item, and no conflicts are known now. At 5ccff7f one defect remains, the cleanup logging, and two things stay open before merge. The failed shutdown or dispose outcome from closeProviders never reaches daemon.log. closeProviders reports it with bare emitDiagnostic. It runs at the lock-lost exit (770), the startup-failure catch (869) and shutdown() (940). None of these is inside withDaemonDiagnosticsScope, and emitDiagnostic returns at once without a scope (host-kit/src/internal/diagnostics.ts:141-142). The same file says so at line 898. So when a plugin's runtime.shutdown or dispose throws on SIGTERM, SIGINT, idle reap or startup failure, daemon.log gets no record, and the provider_plugin_cleanup_failed claim in the PR does not hold. The rule: every daemon event emitted outside a request goes through emitDaemonDiagnostic(logPath, ...), which makes the scope, as provider_plugin_failed already does at line 839. Please use it in closeProviders, and decide on purpose what the lock-lost path does, since that daemon does not own the log. Then add a test in daemon-runtime-metadata-ownership.test.ts. Let shutdownProviderRuntimes resolve to one outcome with shutdown {status:'failed'}, call runtime.shutdown(), and assert that logEvents(stateDir) holds the phase with provider, source and the error code. That test fails at head. Not blocking: the new ./discovery export in packages/plugins/package.json has no importer of @agent-device/plugins/discovery, so you can drop it until a cross-package consumer exists, or keep it. Smoke Tests is still running, and no failure is captured yet. The job covers daemon start and stop through createDaemonRuntimeDeps and closeProviders, so a failure there would likely relate to this PR. Before merge, fix the logging above, land #3748 through #3773 (it is not in this PR, and an unclaimed provider such as testmu still passes assertProviderRuntimeAvailable in lease.ts:340-346), rebase, and get Smoke Tests green. I ran no tests, so regression validity is judged by reading. The real-key Limrun run was on 2dd219b, before this rework. Shutdown now calls the real runtime directly, and no live run covers that at 5ccff7f. On the open threads, one P2 still applies: test harness owner inference is wrong, so use this link instead: #3767 (comment). It is test-only and not blocking. No lower-priority threads still apply. These do not apply and you can resolve them: #3767 (comment) (a test now covers one plugin failing to shut down), #3767 (comment) (a test covers the primed store reuse), #3767 (comment) (withPluginConnection returns the primed connection first), #3767 (comment) (the CLI renders the unavailable message when name is undefined), #3767 (comment) (plain errors get the typed reason plugin_load_failed), #3767 (comment) (the untrimmed Limrun fingerprint is intentional and stated in the PR body), #3767 (comment) (a test-only helper whose doc block states its scope), #3767 (comment) (production validates owners through assertProviderRuntime). |
Built-in providers, the plugin store and an embedder now register through one registry in one shape. A plugin that fails to load, validate or set up is recorded in a daemon inventory and no longer stops the daemon. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…plugins on provider-not-registered Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
5ccff7f to
b55c985
Compare
|
Pushed Applied from the 10:39Z review
Live Limrun run on this head ( |
|
The PR is ready at b55c985. The fix for the earlier findings (#3767 (comment)) is in, and I found no new problems that need a change. Not blocking, and you can take or leave these: The cubic-dev-ai P2 thread on I read the code and ran no tests. The Limrun run on b55c985 is your report only, and no run exercises the failed-cleanup or lock-lost stderr paths. I reviewed only the b55c985 commit as the delta, not the upstream rebase content. Smoke Tests now passes at b55c985, and there are no conflicts. |
…ment * origin/main: feat(provider-testmu): publish @agent-device/testmu on plugin contract v2 (#3770) docs(plugins): document plugin contract v2, the inventory and ADR 0034 (#3769) feat(daemon): daemon status lists the registered provider plugins (#3768) feat(plugins): plugin contract v2 and one provider registry (#3767) # Conflicts: # packages/daemon/src/sessions/handlers/lease.ts # src/__tests__/cloud-connect-testmu.test.ts # src/cli/commands/connection.ts # src/cli/commands/daemon.ts # src/daemon-client/daemon-client-metadata.ts # src/daemon-client/daemon-client.ts
…-no-process-globals * refactor/host-environment: feat(provider-testmu): publish @agent-device/testmu on plugin contract v2 (#3770) docs(plugins): document plugin contract v2, the inventory and ADR 0034 (#3769) feat(daemon): daemon status lists the registered provider plugins (#3768) feat(plugins): plugin contract v2 and one provider registry (#3767) # Conflicts: # packages/daemon/src/server/http/daemon-runtime-composition.test.ts # packages/daemon/src/server/http/daemon-runtime-metadata-ownership.test.ts # packages/daemon/src/server/http/daemon-runtime.ts # src/daemon-composition.ts
Summary
Part 1 of 4 of #3755 (umbrella #3747). Blocked by #3748: do not merge before it.
One plugin contract and one provider registry. Built-in providers, store plugins and (later) embedder plugins register through the same shape, in that order.
agent-device/pluginsexports the type-only v2 contract:AgentDevicePluginFactory,{ apiVersion: 2, provider, credentials, connection?, setup(host) }.{ webDriver }stays a data descriptor../agent-deviceexport is resolved with import conditions. The root export is used only when that subpath is absent. A throwing subpath never falls back.duplicate_provider.cloudandproxystay reserved.active/failed, typederror.reason, credentials withbyBackend) is published indaemon.json. The client fingerprint reads variable names from it and falls back to v1 manifests.RESERVED_PLUGIN_PROVIDERSandDEFAULT_PROVIDER_RUNTIME_REQUIRED_IDS; their consumers (provider policy, fingerprint, connect capabilities, daemon composition) move in this PR.setup) before reading them.Also: a Limrun fingerprint now hashes untrimmed values, like the other providers. 59 files; ~3k gross lines, mostly the replaced loader and its tests.
Stack: this →
feat/plugin-registry-v2-status(daemon status) → docs/ADR 0034 →@agent-device/testmu.Validation
pnpm check:affected --run --head a304bb7f40(the stack top, which contains this layer): passed (exactCommit). Packed-types consumer type-check added to the installed-package test. CI owns integration, coverage and device lanes.🤖 Generated with Claude Code