refactor(plugins): make the plugin framework provider-independent by thymikee · Pull Request #3722 · callstack/agent-device

thymikee · 2026-10-16T14:23:05Z

Summary

Makes @agent-device/plugins independent of concrete providers. Composition supplies reserved provider IDs and WebDriver factory-result adaptation through a required policy; daemon loading, CLI connections and installation share it. The released { webDriver: options } format and agent-device/plugins/webdriver types remain supported.

Plugin-to-provider graph edges fall from 2 to 0. One shared walk (package-isolation.ts) enforces R83 test-kit isolation and R85 plugin independence: manifest reach (direct and transitive), imports including Vitest mock literals and import-bound aliases, relative escapes and reference cycles.

Known conservative contract: the Vitest mock-site reader has no scope engine. An imported vi/vitest alias or namespace counts everywhere in the file, as the global vi already does. For these recognized literal calls, a shadowing local is conservatively counted. Receivers copied, destructured or reached through globalThis/require/dynamic import are not followed.

Validation

Rebased onto origin/main 4c1c786968; head 89825a5449612b43c6a0f59cd1dbb704a95d13c2; 29 files. pnpm 11.17.0, AGENT_DEVICE_VITEST_MAX_WORKERS=1, shared gate lock held throughout.

pnpm check:affected --run --head 89825a5449: exit 0, exactCommit=true, clean tree. 36 passed, 25 skipped, 2 deduplicated. Vitest: 1,871 files, 15,761 tests passed, 1 skipped. Planted static, type-only, dynamic and shadowed-alias provider imports each fail R11 and R85; a kernel import passes. A fresh read-only review found no blockers.

CI owns native, device, provider-integration and coverage checks. No live device validation. All 23 check contexts completed on 89825a5449: 21 passed and 2 expected skips, including successful iOS, macOS, Android and Linux smoke tests, coverage and integration. The earlier iOS Smoke failure belonged to old head 1a1c94f662; no separate native fix was made.

github-actions · 2026-10-16T14:25:23Z

Size Report

Metric	Base	Current	Diff
Installed (including dependencies)	5.32 MB	5.32 MB	+333 B
Package (unpacked)	5.32 MB	5.32 MB	+333 B
Package (download)	1.60 MB	1.60 MB	-4 B

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	26.1 ms	26.3 ms	+0.2 ms
CLI --help	75.4 ms	75.9 ms	+0.5 ms

cubic-dev-ai

All reported issues were addressed across 21 files

_{Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic}

github-actions · 2026-10-16T14:26:48Z

PR Preview Action v1.8.1
🚀 View preview at https://callstack.github.io/agent-device/pr-preview/pr-3722/
Built to branch `gh-pages` at 2026-10-16 19:40 UTC. Preview will be ready when the GitHub Pages deployment is complete.

thymikee · 2026-10-16T14:38:19Z

The plugin changes look good, but the new layering rule R85 needs to reuse the existing isolation helpers before this can merge (reviewed at 524c074).

R85 in plugin-framework-independence.ts copies the forbidden-dependency code from test-kit-isolation.ts almost line for line. The DependencyManifest type, declaredNames, the package-name function and the specifierSites sweep all match. The copy is also weaker. test-kit-isolation follows declared dependencies transitively, but R85 reads only the direct entries in packages/plugins/package.json. So a chain such as plugins -> host-kit/contracts -> provider-* would pass R85, even though its header says the framework takes no provider dependency. There are now two versions of the "this package may not build on packages/provider-*" rule, and they have already diverged. The rule should be: a package declared isolated from a directory class may not reach any package in that class through its manifests, transitively, or through an import site. Please put that rule in one shared helper (the name helpers plus a reach check over the manifests, taking the forbidden directory pattern), and add packages/plugins to it as a second entry instead of keeping a separate file.

Not blocking, and you can take or leave these: the import sweep at line 75 repeats R11, which already rejects imports of undeclared workspace packages, so only the manifest check needs to stay; and changePlugin in store.ts takes a bare reservedProviders array while its siblings take PluginLoadPolicy, which forces plugins.ts:24 to reach into PROVIDER_PLUGIN_POLICY.reservedProviders.

Could the new 90-line R85 file go away entirely? The production side is already small, with one 45-line policy module and a loader that takes the policy as a parameter. A shared table such as { 'packages/test-kit': /platform-|provider-|daemon/, 'packages/plugins': /provider-/ } over the existing BFS and name helpers would cover both packages, with R11 left to guard import sites. That needs the BFS and forbidden-name helpers moved out of test-kit-isolation.ts into a shared layering module first.

Two open threads still apply. The backtick vi.mock gap in package-boundaries.ts should be fixed in the shared specifierSites, not inside R85. The release-assertion test still names a release it never observes.

CI is still running: 11 checks are queued or in progress and none has failed. Repo Guards (R85 and root-module-zones) and Lint/Typecheck run this diff directly, and Smoke and Integration cover daemon provider composition. I read the diff only and did not run the depgraph, affected or layering checks. I also did not confirm whether a transitive plugins -> provider path exists today. The gap above is in what the rule enforces, not a known violation. There are no conflicts. After R85 reuses the shared helpers, the next step is to wait for CI.

cubic-dev-ai

All reported issues were addressed across 6 files (changes from recent commits).

_{Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic}

cubic-dev-ai

All reported issues were addressed across 5 files (changes from recent commits).

_{Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic}

thymikee · 2026-10-16T15:40:06Z

The plugin framework change looks good, but I found one problem to fix before merge. I reviewed 0d46f32.

The new gate scripts/layering/plugin-framework-independence.ts (https://github.com/callstack/agent-device/blob/0d46f32/scripts/layering/plugin-framework-independence.ts#L23) copies the manifest parsing and forbidden-name logic from test-kit-isolation.ts almost line for line. The copy checks only direct entries in packages/plugins/package.json. test-kit-isolation.ts walks workspace manifests transitively. So a chain like plugins -> host-kit -> provider-x would pass this gate, even though the header says the framework takes no provider dependency. The repo now has two versions of one rule, and the plugins version is the weaker one. The earlier blocking request at 524c074 is still unanswered, and the later commits did not touch this file. The rule to enforce is that a package declared isolated from a directory pattern may not reach any package matching that pattern through its manifests, directly or transitively. Please move the manifest parsing, the forbidden-name derivation and the reach walk out of test-kit-isolation.ts into a shared layering module driven by a table, for example { 'packages/test-kit': /platform-|provider-|daemon/ (+root), 'packages/plugins': /provider-/ }. Then delete plugin-framework-independence.ts and its test. Import sites need no extra sweep, because R11 (package-boundaries.ts) already rejects imports of workspace siblings that the manifest does not declare.

Could the change be smaller? The production part (about 45 lines of policy plus the loader taking it) looks right. Could the extractor changes keep only the small specifierSites fix for no-substitution template and computed-literal mock arguments? The import-alias and scope-shadowing resolution (visitAstScoped and the scope helpers in layering-ast.ts, plus vitestImportBindings and isMockApiReceiver in model.ts, about 215 lines together) guards a pattern I could not find in the repo. A grep at this commit finds no import { vi as ... } and no import * as ... from 'vitest' outside scripts/layering. R11 also covers the bypasses that machinery was added for. Dropping it would cut the diff from about 1,200 lines to about 600.

The open inline thread on mockActual (#3722 (comment)) still applies: Vitest has no vi.mockActual, so please remove it from MODULE_MOCK_METHODS in model.ts.

CI is still running. No check has failed. Smoke Tests and Coverage overlap this diff, so I did not wait for them. I did not run the layering gate, the shutdown mutation or the new extractor tests, so the regression checks are reasoned from the diff. Before merge, please replace plugin-framework-independence.ts with the shared table-driven check and answer the 524c074 request.

thymikee · 2026-10-16T16:34:38Z

Addressed in 1f23138c1d (replies to both 524c comment and 0d46 reiteration).

Consolidation (required). scripts/layering/package-isolation.ts now owns manifest parsing, the forbidden-name set (derived from directories, so @agent-device/testmu counts), the breadth-first manifest reach with chain evidence, the import sweep, relative escapes and reference cycles. A policy table drives it: test-kit (platform-, provider-, daemon, root) and plugins (provider-). test-kit-isolation.ts keeps only the single-entry and export * checks and delegates the rest. plugin-framework-independence.ts and its test are deleted; rule IDs R83/R85 and the plugin-framework-independence registry entry are unchanged.

Evidence.

Planted plugins -> host-kit -> provider-webdriver: the old R85 module returns []; the shared walk reports the full chain. On the real gate the plant also trips R83 for test-kit.
The 12 pre-change test-kit tests pass unchanged against the new implementation (single entry, exports, relative escapes, root/platform/provider/daemon exclusion, direct and transitive cycles). Shared cases now run under both policies, including a diamond and foundation negatives.
I kept the import sweep. Planted static, vi.mock and aliased-vi imports of a provider in plugins trip both R11 (undeclared) and R85, so R85 is redundant there. It is the single shared implementation, and it is the only check of root agent-device imports for test-kit (R11 ignores unscoped specifiers). Dropping it for plugins alone would need a per-policy flag.

Optional alias/shadow simplification: not taken (correction in 8a1a6f9ed5). My earlier reply here removed the scope-shadowing walk and accepted over-reporting. That was wrong: a local object such as function f(mocks) { mocks.mock('<provider>') } is not the Vitest mock API, and the over-report produced false provider edges in both R11 and R85. The walk (about 113 lines in layering-ast.ts plus its closest negatives) is restored unchanged from 65ded5558c, and package-isolation.test.ts gains an end-to-end negative for a shadowed alias, paired with the unshadowed alias still refused. R11 shares the same reader, so it cannot replace correct binding. Import-bound vi/vitest aliases and namespaces stay, for the reasons below. The transitive consolidation is unchanged.

Optional reservedProviders store array: not changed. The policy already reaches every entry point as a required argument. A second shape adds API without a failure it prevents.

Validation at 1a1c94f662 (supersedes 8a1a6f9ed5): pnpm check:affected --run --head 1a1c94f662, pnpm 11.17.0, 1 worker, exit 0, exactCommit=true, 36 passed / 25 skipped / 2 deduplicated, 15,602 Vitest tests passed. The three same-name var placements I earlier called exotic (parameter-initializer vs body var, switch discriminant vs case binding, static-block var) were real static-scope bugs and are fixed in 1a1c94f662, with a namespace-body var boundary, each with a positive and a negative test; the new tests fail on 8a1a6f9ed5. Remaining non-goals: with, eval and runtime alias copies are not modelled.

cubic-dev-ai

All reported issues were addressed across 11 files (changes from recent commits).

_{Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic}

cubic-dev-ai

All reported issues were addressed across 5 files (changes from recent commits).

_{Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic}

thymikee · 2026-10-16T18:21:43Z

The iOS failure is still unresolved. It is testSynthesizedReplacementLeavesAMaskedFieldUnverified at the prefilled-value assertion: 13 mask bullets instead of 8. The clear logged passes=2 outcome=unverified before typing.

I compared the PR head 1a1c94f662 with merge-base 9ca333d5fb: the complete apple/, packages/platform-apple/ and .github/ trees are identical. This points away from the provider refactor as the cause, but does not prove a known baseline flake.

The exact test passed all six diagnostic iterations locally on Xcode26.2/iOS26.2; CI used Xcode26.6/iOS26.5. No matching-environment baseline failure was reproduced. Those passes do not replace the failed CI result. The diagnostic simulator was removed, with no sessions left open.

No CI rerun, test weakening or runner patch was made. Further work should reproduce retained text through the existing dropped-select-all test seam and fix the owning clearing invariant separately from this package refactor; merely increasing retries is not yet a proven fix.

thymikee · 2026-10-16T18:39:55Z

The code at 1a1c94f is not ready to merge yet, because the scope-walk simplicity question is still open. The earlier review at 0d46f32 (#3722 (comment)) asked for a shared package-isolation table, and that is now in place. The three cubic-dev-ai threads below are fixed at this head.

The required evidence is in. I did not run the layering gate, the package-isolation tests or check:affected, and I did not check whether the iOS masked-field failure reproduces on main. I reasoned about the scope-boundary tests from the 8a1a6f9 code.

Not blocking, take or leave: the header in https://github.com/callstack/agent-device/blob/1a1c94f/scripts/layering/package-isolation.ts#L17 says a new package is one more row in ISOLATION_POLICIES, but no such constant exists, so either export it and drive the plugin rule and the test from it, or reword the comment.

Can the restored scope walk in https://github.com/callstack/agent-device/blob/1a1c94f/scripts/layering/layering-ast.ts#L236 go? a1fd74b dropped it because no production file aliases vi. 8a1a6f9 brought it back, and 1a1c94f added four more scope-boundary special cases after review found it under-reporting. Meanwhile model.ts:385 treats a shadowed global vi as a receiver, so the model is not consistent, and with and eval are still out of scope. If an import-bound vi or vitest alias or namespace counts everywhere, like the global vi already does, a shadowing local can only over-report. That would remove visitAstScoped and its helpers plus the scope tests, about 300 lines, and the production policy and shared table stay. If over-reporting a shadowed alias is a problem, which file needs the precision?

On the other threads, cubic-dev-ai's three P3/P2 items are fixed at this head and can be resolved: the ParsedManifests change in test-kit-isolation.ts (#3722 (comment)), the childScope var handling (#3722 (comment)), and the missing mockActual method (#3722 (comment)). The childScope fix adds to the scope machinery that the question above asks about.

Smoke Tests fails in the iOS runner XCTest step (testSynthesizedReplacementLeavesAMaskedFieldUnverified, 13 mask bullets where 8 are expected, on Xcode 26.6 and iOS 26.5). This looks unrelated. The diff touches nothing under apple/, packages/platform-apple/ or .github/, and the changed plugin paths are not loaded by that step. It is not yet shown to be a known flake.

Before merge, please answer the simplicity question by dropping the scope walk or saying why it is needed. Then get Smoke Tests green, through a rerun or the separate masked-field fix.

…m the root The plugin loader no longer imports or lists provider-webdriver. The reserved provider ids and the { webDriver } factory-result adaptation move to src/provider-plugin-policy.ts, which every plugin load, connection and installation now receives explicitly.

R85 rejects manifest entries and static, type-only, dynamic and mocked imports from packages/plugins to any packages/provider-* package, and declares the new root policy module's zone.

…onnection

specifierSites matched only quoted vi.mock-style arguments by regex, so a template-literal, computed-member, importMock or doUnmock provider mock bypassed R85. The shared extractor now parses mock-family calls, which also stops comments from counting as sites.

…module sites parseModuleMockSpecifiers matched only the global vi/vitest receivers, so import { vi as mocks } from 'vitest' or a vitest namespace import hid a provider mock from R85 and every other specifierSites walk. The shared extractor now resolves top-level value imports from vitest (named vi/vitest aliases and namespace members), honours lexical redeclaration of those local names, and still ignores type-only imports, other modules and other exports. Bindings made by a dynamic import of vitest are not followed.

…thod set Vitest exposes importActual, not mockActual. The unsupported name made R85 and every specifierSites walk flag a call no test can make.

…reader An alias a local redeclares now counts as the alias, so the reader can only over-report. Import-bound aliases and namespaces stay resolved, and no production file aliases vi, so the scope walk bought no coverage.

…he plugin framework R83 test-kit isolation and R85 plugin-framework independence each parsed manifests and swept imports. Manifest parsing, the forbidden-name set, the dependency-reach walk and the import sweep now live in package-isolation.ts behind a per-package policy table. R85 gains the transitive manifest reach it lacked: plugins -> host-kit -> a provider package passed the direct-dependency check.

A local that redeclares an imported Vitest alias is not the mock API; the shared isolation walk must not report a provider edge for it.

…ation walks The test-kit shape check and the shared isolation walk each parsed every workspace manifest. The walk now takes the parsed map, so the caller parses once at the boundary.

A parameter initializer, a switch discriminant, a class static block and a namespace body each have a scope boundary the reader flattened into the enclosing scope, so a later declaration hid a real mock of a provider.

…cking An imported vi/vitest alias or namespace now counts everywhere in the file, as the global vi and vitest already do. A local that shadows an alias name is over-reported; no mock call is missed. Drops visitAstScoped and its binding machinery.

…er does not follow

thymikee · 2026-10-16T19:39:22Z

Agreed — took the simplification at 89825a5449.

Removed visitAstScoped and all binding/hoisting helpers from layering-ast.ts (147 helper lines removed; net 234 lines removed across five files). model.ts uses plain visitAst; the scope tests are gone.
Contract: an imported vi/vitest alias or namespace counts everywhere in the file, like the global vi. For these recognized literal calls, a shadowing local is conservatively counted. No production file uses an alias, so precision had no consumer. A short test now pins the shadowed-alias case as intentionally counted.
Kept: the shared isolation table, transitive map/BFS, ref-cycle, export and relative-escape checks, and manifest reuse. The package-isolation.ts header now points at PackageIsolationPolicy, not a nonexistent table.
Proof: planted static, type-only, dynamic and shadowed-alias provider imports in packages/plugins/src each fail R11 and R85; a kernel import passes.
Gate: pnpm check:affected --run --head 89825a5449 passed (exit 0, exactCommit, 36 passed/25 skipped; 15,761 tests). Rebased on 4c1c786968.
iOS failure was on the old head; new CI is pending.

thymikee · 2026-10-16T22:44:57Z

I found no code problems at 89825a5. The earlier points are fixed, and the simplicity challenge is answered by deleting the scope walk.

All 23 checks have completed on this commit and none failed. The iOS Smoke XCTest failure was on the old head 1a1c94f. This diff touches no apple/, packages/platform-apple/ or .github/ path, so that failure does not apply here. There are no conflicts. Nothing else stands in the way, so this is ready for a maintainer to decide on merge.

I judged behavior by reading the head source and the test expectations. I did not run the layering gate, the package-isolation tests, the model-mock-bindings tests or check:affected. I also did not reproduce the planted-import proof (static, type-only, dynamic and shadowed-alias provider imports failing R11/R85). I reviewed only the logical delta from the rebase, 759e348..89825a5.

The six Cubic threads are all fixed at this head, so please resolve them. Each one's fix is already in the code at this head:

visitAstScoped is deleted, and the default-parameter initializer case is now counted: refactor(plugins): make the plugin framework provider-independent #3722 (comment)
mockActual is no longer missed, and importActual is expected as ['d','e','f']: refactor(plugins): make the plugin framework provider-independent #3722 (comment)
The workspace manifests are parsed once for both isolation walks, in 3a2b428: refactor(plugins): make the plugin framework provider-independent #3722 (comment)
vitestImportBindings resolves import { vi as x } and namespace imports, and the alias counts file-wide: refactor(plugins): make the plugin framework provider-independent #3722 (comment)
literalSpecifier accepts template literals: refactor(plugins): make the plugin framework provider-independent #3722 (comment)
The runtime release after a WebDriver plugin connection is observed in 02bbf31: refactor(plugins): make the plugin framework provider-independent #3722 (comment)

thymikee · 2026-10-17T06:10:59Z

Confirmed on 89825a5449612b43c6a0f59cd1dbb704a95d13c2: all six linked Cubic threads are already resolved. I rechecked their state after this review; none remain open.

All 23 check contexts completed: 21 passed and 2 expected skips, including iOS Smoke, Coverage and Integration Tests. The PR is non-draft and conflict-free. The PR body records the exact-head affected-gate proof and final CI result. The earlier iOS failure remains attributed to 1a1c94f662; no separate native fix or manual rerun was made.

No further code changes are needed for this review. Ready for a maintainer's merge decision.

…ment * origin/main: feat(cli): ensure the daemon explicitly before attaching in the CLI and MCP (#3766) feat(client): daemonLifecycle attach mode and ensureDaemon() (#3765) feat(daemon): quiesce request that retires an idle daemon atomically (#3764) fix(daemon): answer a named missing session on the session, not the device (#3701) test: route test/ and scripts/ off the daemon's testing exports (#3733) refactor(plugins): make the plugin framework provider-independent (#3722) fix(apple): name an Xcode too old for its host macOS as a runner startup failure (#3739) # Conflicts: # packages/daemon/src/server/http/daemon-runtime.ts # packages/daemon/src/sessions/handlers/session-appstate-device-runtime.ts # packages/daemon/test/nightly/concurrency-torture/real-scope-serialization.ts # src/__tests__/daemon-registration-owner.test.ts # src/agent-device-client.ts # src/cli.ts # src/cli/commands/proxy.ts # src/daemon-client/daemon-client-lifecycle.ts # src/daemon-client/daemon-client-metadata.ts # src/daemon-client/daemon-client.ts # test/integration/provider-scenarios/harness.ts

…refactor/daemon-runtime-no-process-globals * origin/refactor/host-environment: refactor(daemon): read configuration from the request host instead of the ambient scope test: merge the duplicate test-kit import in the stale provider admission scenarios feat(cli): ensure the daemon explicitly before attaching in the CLI and MCP (#3766) feat(client): daemonLifecycle attach mode and ensureDaemon() (#3765) feat(daemon): quiesce request that retires an idle daemon atomically (#3764) fix(daemon): answer a named missing session on the session, not the device (#3701) test: route test/ and scripts/ off the daemon's testing exports (#3733) refactor(plugins): make the plugin framework provider-independent (#3722) fix(apple): name an Xcode too old for its host macOS as a runner startup failure (#3739) # Conflicts: # packages/daemon/src/server/http/daemon-runtime.ts