feat(omnium): Add controller architecture #752
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| throw new Error('Failed to determine subclusterId after launch'); | ||
| } | ||
|
|
||
| return { subclusterId: newSubcluster.id }; |
There was a problem hiding this comment.
Race condition in concurrent subcluster ID identification
Medium Severity
The launchSubcluster wrapper identifies newly created subclusters by comparing status snapshots before and after launch. When multiple caplets are installed concurrently, all callers may capture the same "before" snapshot, launch their subclusters, then each use find() on the "after" snapshot. Since find() returns the first match, concurrent installs could identify the same subcluster as "new", causing incorrect subclusterId associations and orphaned subclusters. On uninstall, this could terminate the wrong subcluster.
| subclusterId, | ||
| installedAt: Date.now(), | ||
| }; | ||
| }); |
There was a problem hiding this comment.
TOCTOU race allows duplicate caplet installations
Medium Severity
The #install method checks if a caplet is already installed (line 204) before an await call to launchSubcluster (line 219), then updates state afterward (line 221). Two concurrent install calls for the same caplet ID can both pass the "already installed" check before either updates state. This results in two subclusters being created for the same caplet, with the second update() overwriting the first, orphaning one subcluster.
|
|
||
| if (!newSubcluster) { | ||
| throw new Error('Failed to determine subclusterId after launch'); | ||
| } |
There was a problem hiding this comment.
Missing cleanup orphans subcluster on identification failure
Medium Severity
After launchSubcluster succeeds (line 142), if the subsequent getStatus() call fails or returns unexpected results causing the "Failed to determine subclusterId" error (line 151), the already-created subcluster is left running with no reference to it. There's no try-catch with cleanup logic to terminate the orphaned subcluster on failure. This creates a resource leak where subclusters accumulate in the kernel with no way to clean them up.
rekmarks
force-pushed
the
rekm/controller-architecture
branch
from
c98290e to
6736065
Compare
Implement Phase 1.2 of the omnium plan - Define Caplet Structure: - Add modular controller architecture with POLA attenuation via makeFacet() - Add storage abstraction layer (StorageAdapter, NamespacedStorage) - Add Chrome storage adapter for platform storage - Add CapletController for managing installed caplets - Add Caplet types with superstruct validation - Wire CapletController into background.ts and expose on globalThis.omnium.caplet - Add comprehensive unit tests for all controller code - Update PLAN.md to reflect implementation
Consolidate CapletControllerState from multiple top-level keys (installed, manifests, subclusters, installedAt) into a single `caplets: Record<CapletId, InstalledCaplet>` structure. Changes: - Add ControllerStorage abstraction using Immer for state management - Controllers work with typed state object instead of storage keys - Only modified top-level keys are persisted (via Immer patches) - Remove state corruption checks (no longer possible with atomic storage) - Fix makeFacet type - use string | symbol instead of keyof MethodGuard - Update PLAN.md to reflect new storage architecture 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add abstract Controller class with state management via ControllerStorage - Convert CapletController to extend Controller base class - Use makeFacet() pattern for returning hardened exo methods - Add base-controller tests (12 tests) - Add semver deep import type declaration - Add storage permission to manifest.json Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Convert ControllerStorage from factory to class with static make() method - Implement synchronous update() with debounced fire-and-forget persistence - Fix critical debounce bug: accumulate modified keys across debounce window - Implement bounded latency (timer not reset, max delay = one debounce interval) - Add immediate writes when idle > debounceMs for better responsiveness - Add clear() and clearState() methods to reset storage to defaults - Remove old namespaced-storage implementation - Refactor all tests to use actual ControllerStorage with mock adapters - Add shared makeMockStorageAdapter() utility in test/utils.ts - Update controllers to create their own storage from adapters Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Remove strict reverse DNS format requirement for CapletId to allow more flexibility during early development. Now accepts any non-empty ASCII string without whitespace, removing restrictions on hyphens, underscores, uppercase, and segment count. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rekmarks
force-pushed
the
rekm/controller-architecture
branch
from
6736065 to
89e5113
Compare
Summary
Test plan
Part 2 of 4 in PR stack for omnium-III
Depends on: #751
🤖 Generated with Claude Code
Note
Introduces a controller system with persistent state and caplet lifecycle management, and wires it into the extension background.
Controller,ControllerStorage(Immer-based, debounced per-key persistence), and POLAmakeFacethelperCapletControllerwithinstall/uninstall/list/get/getByService, manifest validation (@metamask/superstruct,semver), and subcluster launch/terminate hooksomnium.capletAPI inbackground.tsusing attenuated kernel methods and a Chrome storage adapter (makeChromeStorageAdapter)global.d.tstypes and updatesmanifest.jsonpermissions to includestorage@endo/exo,@metamask/*,immer,semver,@types/semver)Written by Cursor Bugbot for commit c98290e. This will update automatically on new commits. Configure here.