chore(apps): consolidate accessor implementation to runtime (3/4) (#41378)

Author: d-gubert

docs/proposals/apps-accessor-consolidation/README.md

@@ -485,9 +485,16 @@ since consolidating more logic into the runtime makes the assumption more load-b
    dropping.
 3. **`messenger.sendRequest` timeout** — the runtime-side TODO becomes more prominent once all
    accessor traffic flows through it.
-4. **Drop the legacy `_accessors` threading** through `AppApi.runExecutor`,
-   `AppSlashCommand.runTheCode`, `AppVideoConfProvider.runTheCode`,
-   `AppOutboundCommunicationProvider.runTheCode` signatures.
+4. ✅ **Drop the legacy `_accessors` threading + delete the dead host accessor layer.** — **landed as
+   Phase 5** (§8). Un-threaded the ignored `accessors: AppAccessorManager` parameter from
+   `AppApi.runExecutor`, `AppSlashCommand`/`AppVideoConfProvider`/`AppOutboundCommunicationProvider`
+   `run*`/`runTheCode` and their managers; deleted `AppAccessorManager` (+ its `getAccessorManager()`
+   on `AppManager` and the `purifyApp` call), the entire `src/server/accessors/` directory (unreachable
+   from the subprocess after Phase 4), the host `src/server/misc/UIHelper.ts` copy (its only importers
+   were those deleted accessors), and the `managers/index.ts` export. Behavior-neutral cleanup; kept
+   separate from the Phase 4 message-path teardown because it touches the sandbox-execution core.
+   (The parity harness is **not** removed — it became a permanent test utility the base-runtime
+   accessor tests depend on.)
 5. **Consolidated host↔subprocess protocol/SDK** — a single typed manifest of every host-bound method
    and its accepted params (the §5.2 explicit method/exception list is the seed), replacing today's
    hand-rolled message strings and per-param normalization judgment with a declared contract. A
@@ -672,21 +679,79 @@ returning the bridge value, matching the interface.
 **RPC-boundary note:** `SettingRead.getValueById` treats `null` and `undefined` alike as "does not
 exist" (undefined serializes to null across the boundary), same adaptation as `ServerSettingRead`.
 
-### Phase 4 — Teardown
-
-1. Replace `AppListenerManager.executePostMessageSent`'s `getReader(...).getAppUser()` with
-   `bridges.getUserBridge().doGetAppUser(appId)`.
-2. Delete `handleAccessorMessage`, `ALLOWED_ACCESSOR_METHODS`, `isValidOrigin`,
-   `getAccessorForOrigin`, and the `accessor:` branch in `handleIncomingMessage`.
-3. Delete `src/server/accessors/` entirely (including dead `AppAccessors`, `Http`), delete
-   `AppAccessorManager` (and its `purifyApp` call in `AppManager`), remove `proxify` from `mod.ts`,
-   and drop the now-unused `getAccessorManager()` threading in managers (follow-up #4 can ride
-   along).
-4. Delete the `src/server/misc/UIHelper.ts` copy (its last importers are gone with the host
-   accessors), leaving the base-runtime copy as the single source of truth; remove the transitional
-   parity harness (§6).
-5. CHANGELOG entry; update any architecture docs referencing the accessor message category.
-
-**End state:** `BaseRuntimeSubprocessController` handles exactly one app-originated RPC category —
-`bridges:*` — with a single dispatcher, a single permission model, and a single accessor
-implementation living in `packages/apps/base-runtime`.
+### Phase 4 — Teardown — ✅ landed (message-path teardown; dead-class deletion moved to follow-up #4)
+
+1. ✅ Replaced `AppListenerManager.executePostMessageSent`'s `getReader(appId).getUserReader().getAppUser()`
+   with `this.manager.getBridges().getUserBridge().doGetAppUser(appId)` — the only load-bearing
+   host-side accessor consumer outside the RPC path. `AppListenerManager` no longer references
+   `AppAccessorManager`.
+2. ✅ Deleted `handleAccessorMessage`, `ALLOWED_ACCESSOR_METHODS`, `isValidOrigin`,
+   `getAccessorForOrigin`, the `accessor:` branch in `handleIncomingMessage`, and the controller's
+   now-unused `accessors`/`api` fields (and their `manager.getAccessorManager()`/`getApiManager()`
+   reads). `proxify` and `WithProxy` were already removed from `mod.ts` in Phase 3. **The
+   `accessor:*` message category no longer exists — the controller dispatches only `bridges:*`,
+   `ready`, `log`, and the error notifications.** `JSONRPC_METHOD_NOT_FOUND` is kept (it is imported by
+   `ProxiedApp`, `AppListenerManager`, `AppVideoConfProvider`).
+3. ✅ Adapted `tests/server/runtime/DenoRuntimeSubprocessController.test.ts`: removed the four
+   `handleAccessorMessage` cases (HTTP accessor, IRead, IEnvironmentReader-via-IRead, LivechatCreator
+   visitor) that exercised the now-deleted `accessor:*` dispatch path. Their host-side resolution no
+   longer exists (the subprocess resolves accessors locally via `bridges:*`), and the equivalent
+   behavior is covered by the surviving `handleBridgeMessage` "message bridge" case here plus the
+   base-runtime accessor/RemoteBridges suites. The unused `UserStatusConnection`/`UserType` import was
+   dropped with them.
+
+**Scoping decision — physical deletion of the dead accessor classes is folded into follow-up #4.**
+Deleting `src/server/accessors/` + `AppAccessorManager` requires first un-threading the (ignored)
+`accessors: AppAccessorManager` parameter from the sandbox-execution core (`AppApi.runExecutor`,
+`AppSlashCommand`/`AppVideoConfProvider`/`AppOutboundCommunicationProvider` `run*`/`runTheCode`, and
+their managers). That is exactly follow-up #4 ("drop the legacy `_accessors` threading"), which the
+plan already carved out as a separate, behavior-neutral change. Rather than refactor the execution
+core at the tail of this work for zero behavior benefit, the now-**unreachable** host accessor
+classes + `AppAccessorManager` are left in place (dead code — nothing on the subprocess path reaches
+them) and their removal is done in follow-up #4 together with the threading cleanup. The host
+`src/server/misc/UIHelper.ts` copy and the parity harness stay for the same reason (the harness has
+also become a genuine, permanent test utility that the Phase 1–3 tests depend on, so it is retained
+rather than removed).
+
+**End state (primary objectives met):** `BaseRuntimeSubprocessController` handles exactly one
+app-originated RPC category — `bridges:*` — with a single dispatcher and a single permission model,
+and accessor *behavior* lives in exactly one place, `packages/apps/base-runtime`. `handleAccessorMessage`
+and the entire `accessor:*` category are gone. What remains is dead-code removal (Phase 5 / follow-up #4).
+
+### Phase 5 — Dead host accessor layer removal (follow-up #4) — ✅ landed
+
+Physical removal of the now-unreachable host accessor code. Behavior-neutral: nothing on the
+subprocess path reached any of it after Phase 4.
+
+1. ✅ **Un-threaded the vestigial `accessors` parameter** from the sandbox-execution core. The
+   `run*`/`runTheCode` methods already ignored it (`_accessors`) — the app executes in the subprocess
+   via `getRuntimeController().sendRequest`, so the host-side accessor manager was never consulted.
+   Dropped the parameter from `AppApi.runExecutor`, `AppSlashCommand.runExecutorOrPreviewer`/
+   `runPreviewExecutor`/`runTheCode`, all `AppVideoConfProvider.run*`/`runTheCode`, and
+   `AppOutboundCommunicationProvider.runGetProviderMetadata`/`runSendOutboundMessage`/`runTheCode`, and
+   updated the four managers' call sites. Removed the now-unused `private readonly accessors` field
+   (and its `this.manager.getAccessorManager()` assignment) from `AppApiManager`,
+   `AppSlashCommandManager`, `AppVideoConfProviderManager`, and `AppOutboundCommunicationProviderManager`.
+2. ✅ **Deleted `AppAccessorManager`** (`src/server/managers/AppAccessorManager.ts`), removed its
+   `managers/index.ts` export, and removed `AppManager`'s `accessorManager` field, its
+   `new AppAccessorManager(this)` construction, the `getAccessorManager()` accessor, and the
+   `accessorManager.purifyApp(...)` call in `removeLocal` (a no-op — the caches it cleared were never
+   populated once no host code called `getReader`/`getModifier`/etc.).
+3. ✅ **Deleted the entire `src/server/accessors/` directory** (64 files — the host copies of the
+   readers, modifiers, builders, environment accessors, and the configuration/registration accessors)
+   and the host `src/server/misc/UIHelper.ts` (only ever imported by those deleted accessors). The
+   base-runtime holds the single source of truth for all of this.
+4. ✅ **Adapted tests:** deleted `tests/server/accessors/` (35 host-accessor unit tests — the
+   base-runtime suites cover the equivalent behavior) and `tests/server/managers/AppAccessorManager.test.ts`;
+   removed the `AppAccessorManager`/`getAccessorManager()`/`(x as any).accessors` setup and assertions
+   from the four manager tests, `AppSettingsManager.test.ts`, `AppManager.test.ts`, the two Deno-runtime
+   integration tests, and `AppListenerManager.test.ts`.
+
+**Verification:** host `tsc` error-set unchanged vs the environmental-only baseline (no new errors);
+`test:node` host suites green (excluding the two `deno`-spawn integration suites, which require the
+`deno` binary unavailable in this environment); base-runtime suite unaffected (untouched by this phase).
+
+**End state:** the `@rocket.chat/apps` host no longer contains any accessor implementation or accessor
+manager. The only accessor-shaped code left on the host is the bridge layer (`AppBridges`/`do*`) and
+`AppResourceBridge`; every accessor an app actually uses is built and resolved inside
+`packages/apps/base-runtime`.

packages/apps/src/server/AppManager.ts

@@ -17,7 +17,6 @@ import { AppCompiler, AppFabricationFulfillment, AppPackageParser } from './comp
 import { InvalidLicenseError } from './errors';
 import { InvalidInstallationError } from './errors/InvalidInstallationError';
 import {
-	AppAccessorManager,
 	AppApiManager,
 	AppExternalComponentManager,
 	AppLicenseManager,
@@ -87,8 +86,6 @@ export class AppManager {
 
 	private readonly compiler: AppCompiler;
 
-	private readonly accessorManager: AppAccessorManager;
-
 	private readonly listenerManager: AppListenerManager;
 
 	private readonly commandManager: AppSlashCommandManager;
@@ -153,7 +150,6 @@ export class AppManager {
 
 		this.parser = new AppPackageParser();
 		this.compiler = new AppCompiler();
-		this.accessorManager = new AppAccessorManager(this);
 		this.listenerManager = new AppListenerManager(this);
 		this.commandManager = new AppSlashCommandManager(this);
 		this.apiManager = new AppApiManager(this);
@@ -200,11 +196,6 @@ export class AppManager {
 		return this.compiler;
 	}
 
-	/** Gets the accessor manager instance. */
-	public getAccessorManager(): AppAccessorManager {
-		return this.accessorManager;
-	}
-
 	/** Gets the instance of the Bridge manager. */
 	public getBridges(): AppBridges {
 		return this.bridges;
@@ -1096,7 +1087,6 @@ export class AppManager {
 		this.listenerManager.lockEssentialEvents(app);
 		this.externalComponentManager.unregisterExternalComponents(app.getID());
 		await this.apiManager.unregisterApis(app.getID());
-		this.accessorManager.purifyApp(app.getID());
 		this.uiActionButtonManager.clearAppActionButtons(app.getID());
 		await this.videoConfProviderManager.unregisterProviders(app.getID());
 		await this.outboundCommunicationProviderManager.unregisterProviders(app.getID(), {