chore(apps): consolidate accessor implementation to runtime (2/4) - #41377
Merged
Conversation
|
|
Contributor
|
Looks like this PR is ready to merge! 🎉 |
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR replaces base-runtime accessor proxies with concrete bridge-backed implementations, adds read, environment, persistence, and modify accessors, introduces ChangesAccessor consolidation
Estimated code review effort: 4 (Complex) | ~50 minutes Sequence Diagram(s)sequenceDiagram
participant Runtime as Base Runtime
participant AppAccessors
participant RemoteBridges
participant SubprocessController
participant AppResourceBridge
participant Host as Host Manager
Runtime->>AppAccessors: getConfigurationExtend()
AppAccessors->>RemoteBridges: getAppResourceBridge()
RemoteBridges->>SubprocessController: bridges:getAppResourceBridge:doProvideApi
SubprocessController->>AppResourceBridge: dispatch doProvideApi(api, 'APP_ID')
AppResourceBridge->>Host: register API in manager
Host-->>AppResourceBridge: success
AppResourceBridge-->>SubprocessController: void
SubprocessController-->>RemoteBridges: JSON-RPC result
RemoteBridges-->>AppAccessors: apiExtend ready
AppAccessors-->>Runtime: configuration extend result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
d-gubert
changed the title
d-gubert
force-pushed
the
chore/apps-accessor-consolidation-1
branch
from
July 15, 2026 17:59
06c7719 to
477d50c
Compare
d-gubert
force-pushed
the
chore/apps-accessor-consolidation-2
branch
2 times, most recently
from
July 15, 2026 18:49
e1698b5 to
91104bd
Compare
d-gubert
force-pushed
the
chore/apps-accessor-consolidation-1
branch
from
July 15, 2026 19:08
7940049 to
8df5c8d
Compare
d-gubert
force-pushed
the
chore/apps-accessor-consolidation-2
branch
from
July 15, 2026 19:08
91104bd to
9630cd4
Compare
d-gubert
force-pushed
the
chore/apps-accessor-consolidation-1
branch
from
July 16, 2026 13:41
8df5c8d to
6a9ed9d
Compare
d-gubert
force-pushed
the
chore/apps-accessor-consolidation-2
branch
from
July 16, 2026 15:55
9630cd4 to
61c16fd
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #41377 +/- ##
========================================
Coverage 68.71% 68.72%
========================================
Files 4151 4151
Lines 159491 159491
Branches 27999 28025 +26
========================================
+ Hits 109596 109609 +13
+ Misses 44722 44708 -14
- Partials 5173 5174 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
d-gubert
force-pushed
the
chore/apps-accessor-consolidation-2
branch
from
July 16, 2026 18:18
61c16fd to
8bc03d6
Compare
d-gubert
force-pushed
the
chore/apps-accessor-consolidation-1
branch
from
July 16, 2026 18:21
6a9ed9d to
5b74754
Compare
d-gubert
force-pushed
the
chore/apps-accessor-consolidation-2
branch
from
July 28, 2026 18:58
8bc03d6 to
c0d24d2
Compare
d-gubert
marked this pull request as ready for review
July 28, 2026 18:59
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/proposals/apps-accessor-consolidation/README.md (1)
638-638: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the stale
proxifyteardown item.Phase 3 records that
proxifywas already removed frommod.ts; keeping it in the Phase 4 checklist creates a misleading follow-up task.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/proposals/apps-accessor-consolidation/README.md` at line 638, Remove the stale “remove proxify from mod.ts” checklist item from the Phase 4 teardown section, while retaining the AppAccessorManager and AppManager purifyApp entries.packages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts (1)
565-570: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftBind app identity server-side instead of rewriting arbitrary payload values.
Only the
"APP_ID"sentinel is replaced; any other supplied app ID is forwarded. A subprocess can directly request an AppResourceBridge operation with another app’s ID, while legitimate top-level data equal to"APP_ID"—for example a setting value—is silently replaced with the caller ID. Inject/validate identity by bridge-method contract and reject raw caller-identity parameters, with explicit handling only for documented exceptions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts` around lines 565 - 570, Update the method invocation flow around BaseRuntimeSubprocessController’s methodRef.apply call to stop rewriting arbitrary payload values via params.map. Enforce the caller’s app identity server-side according to each AppResourceBridge method contract, rejecting or overriding raw caller-supplied identity parameters while preserving legitimate payload values equal to "APP_ID"; handle only explicitly documented exceptions.
🧹 Nitpick comments (4)
packages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.ts (1)
6-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove implementation comments introduced by this migration.
packages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.ts#L6-L8: remove the forwarding rationale comment.packages/apps/base-runtime/src/lib/accessors/mod.ts#L119-L120: remove the environment wiring comment.packages/apps/base-runtime/src/lib/accessors/mod.ts#L145-L146: remove the slash-command registry comment.packages/apps/base-runtime/src/lib/accessors/mod.ts#L185-L186: remove the API cache comment.packages/apps/base-runtime/src/lib/accessors/mod.ts#L196-L198: remove the processor registry comment.packages/apps/base-runtime/src/lib/accessors/mod.ts#L207-L208: remove the video-provider registry comment.packages/apps/base-runtime/src/lib/accessors/mod.ts#L227-L228: remove the slash-command registry comment.packages/apps/base-runtime/src/lib/accessors/mod.ts#L235-L237: remove theregisterButtonimplementation comment.packages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.ts#L5-L7: remove the placeholder rationale comment.packages/apps/base-runtime/src/lib/accessors/read/LivechatRead.ts#L12-L20: remove theisOnlineimplementation comment.packages/apps/base-runtime/src/lib/accessors/read/LivechatRead.ts#L49-L53: remove the deprecated-method implementation comment.As per coding guidelines, “Avoid code comments in the implementation.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.ts` around lines 6 - 8, Remove the migration-introduced implementation comments while preserving all surrounding behavior: delete the forwarding rationale in packages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.ts lines 6-8; delete the specified environment wiring, slash-command registry, API cache, processor registry, video-provider registry, and registerButton comments in packages/apps/base-runtime/src/lib/accessors/mod.ts lines 119-120, 145-146, 185-186, 196-198, 207-208, 227-228, and 235-237; and delete the placeholder rationale plus isOnline and deprecated-method comments in packages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.ts lines 5-7 and packages/apps/base-runtime/src/lib/accessors/read/LivechatRead.ts lines 12-20 and 49-53.Source: Coding guidelines
packages/apps/base-runtime/src/lib/accessors/read/UserRead.ts (1)
25-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove implementation comments.
packages/apps/base-runtime/src/lib/accessors/read/UserRead.ts#L25-L27: remove the sentinel-behavior comments.packages/apps/base-runtime/src/lib/accessors/read/RoomRead.ts#L102-L102: remove the redundantvalidateSortcomment.As per coding guidelines,
**/*.{ts,tsx,js}must “Avoid code comments in the implementation.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apps/base-runtime/src/lib/accessors/read/UserRead.ts` around lines 25 - 27, Remove the sentinel-behavior implementation comments at UserRead.ts lines 25-27 and the redundant validateSort comment at RoomRead.ts line 102; make no other code changes.Source: Coding guidelines
packages/apps/base-runtime/src/lib/accessors/modify/LivechatCreator.ts (1)
38-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the implementation comment.
The entropy is evident from
randomBytes(16)and the repository guideline prohibits implementation comments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apps/base-runtime/src/lib/accessors/modify/LivechatCreator.ts` at line 38, Remove the inline implementation comment from the randomBytes call in the LivechatCreator identifier-generation logic, leaving the behavior and expression unchanged.Source: Coding guidelines
packages/apps/base-runtime/src/lib/accessors/modify/ModerationModify.ts (1)
7-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove newly added implementation comments.
The repository guideline requests comment-free implementation code; retain this rationale in the accessor-consolidation documentation instead.
packages/apps/base-runtime/src/lib/accessors/modify/ModerationModify.ts#L7-L9: remove the app-ID forwarding comment.packages/apps/base-runtime/src/lib/accessors/modify/SchedulerModify.ts#L7-L9: remove the scheduler-ID comment.packages/apps/base-runtime/src/lib/accessors/modify/UIController.ts#L26-L28: remove the UI app-ID comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apps/base-runtime/src/lib/accessors/modify/ModerationModify.ts` around lines 7 - 9, Remove the newly added implementation comments from ModerationModify.ts lines 7-9, SchedulerModify.ts lines 7-9, and UIController.ts lines 26-28; preserve the surrounding implementation unchanged, with the rationale retained only in the accessor-consolidation documentation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/apps/base-runtime/src/lib/accessors/environment/SettingRead.ts`:
- Around line 6-7: Remove the added implementation comments without changing
behavior: delete the AppResourceBridge comment in
packages/apps/base-runtime/src/lib/accessors/environment/SettingRead.ts at lines
6-7, the RPC null-serialization comment there at lines 18-19, the corresponding
comment in
packages/apps/base-runtime/src/lib/accessors/environment/ServerSettingRead.ts at
lines 16-17, and the getAppResourceBridge JSDoc in
packages/apps/base-runtime/src/lib/bridges/RemoteBridges.ts at lines 157-162.
In `@packages/apps/base-runtime/src/lib/accessors/modify/SchedulerModify.ts`:
- Around line 10-12: Update createProcessorId to detect whether jobId ends with
the app-specific suffix rather than merely containing _${appId}; use an exact
suffix check so names containing the app ID elsewhere still receive the correct
namespace.
In `@packages/apps/base-runtime/src/lib/accessors/modify/UIController.ts`:
- Around line 36-58: The deprecated methods openModalView, updateModalView,
openContextualBarView, and updateContextualBarView must normalize block IDs
before invoking their serializers, matching the behavior of openSurfaceView and
updateSurfaceView. Route these legacy APIs through the same UIHelper.assignIds
flow while preserving their existing modal/contextual-bar and update semantics.
In `@packages/apps/base-runtime/src/lib/accessors/modify/UploadCreator.ts`:
- Around line 12-15: Update uploadBuffer to treat a null descriptor.user as
absent when deciding whether to fetch the app user. Replace the
property-ownership check with a value-based null/absence check, while preserving
the visitorToken bypass and existing app-user assignment behavior.
In `@packages/apps/base-runtime/src/lib/accessors/read/ContactRead.ts`:
- Around line 9-10: Update ContactRead.getById to match the IContactRead
contract by returning Promise<ILivechatContact | null> and casting the bridge
result to that type, preserving the existing contact lookup behavior while
representing missing contacts as null rather than undefined.
In `@packages/apps/base-runtime/src/lib/accessors/read/LivechatRead.ts`:
- Around line 21-27: The deprecated LivechatRead.isOnline method must not return
the promise produced by doIsOnline. Remove this incompatible synchronous
accessor or update it only if a genuine synchronous boolean can be obtained;
otherwise direct callers to isOnlineAsync while preserving the asynchronous
bridge behavior.
In `@packages/apps/base-runtime/src/lib/accessors/read/MessageRead.ts`:
- Around line 15-20: Update
packages/apps/base-runtime/src/lib/accessors/read/MessageRead.ts lines 15-20 and
25-30 so bridge results used by getSenderUser and getRoom are typed as optional,
preserving undefined/null responses in their return and argument types; when
passing the fetched message from IMessageRead.getById, restore the non-optional
contract before calling these methods. Update
packages/apps/base-runtime/src/lib/accessors/read/ThreadRead.ts lines 9-10
similarly to preserve optional bridge results in the relevant accessor type.
In `@packages/apps/base-runtime/src/lib/accessors/tests/ModifyCreator.test.ts`:
- Around line 65-85: Update the ModifyCreator uploadBuffer test to make the
doGetAppUser mock return a user object such as { id: 'app-user' } instead of its
request payload, then assert that the doCreateUpload payload contains userId:
'app-user'. Keep the existing request-order and other payload assertions
unchanged.
---
Outside diff comments:
In `@docs/proposals/apps-accessor-consolidation/README.md`:
- Line 638: Remove the stale “remove proxify from mod.ts” checklist item from
the Phase 4 teardown section, while retaining the AppAccessorManager and
AppManager purifyApp entries.
In `@packages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts`:
- Around line 565-570: Update the method invocation flow around
BaseRuntimeSubprocessController’s methodRef.apply call to stop rewriting
arbitrary payload values via params.map. Enforce the caller’s app identity
server-side according to each AppResourceBridge method contract, rejecting or
overriding raw caller-supplied identity parameters while preserving legitimate
payload values equal to "APP_ID"; handle only explicitly documented exceptions.
---
Nitpick comments:
In `@packages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.ts`:
- Around line 6-8: Remove the migration-introduced implementation comments while
preserving all surrounding behavior: delete the forwarding rationale in
packages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.ts lines
6-8; delete the specified environment wiring, slash-command registry, API cache,
processor registry, video-provider registry, and registerButton comments in
packages/apps/base-runtime/src/lib/accessors/mod.ts lines 119-120, 145-146,
185-186, 196-198, 207-208, 227-228, and 235-237; and delete the placeholder
rationale plus isOnline and deprecated-method comments in
packages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.ts lines 5-7
and packages/apps/base-runtime/src/lib/accessors/read/LivechatRead.ts lines
12-20 and 49-53.
In `@packages/apps/base-runtime/src/lib/accessors/modify/LivechatCreator.ts`:
- Line 38: Remove the inline implementation comment from the randomBytes call in
the LivechatCreator identifier-generation logic, leaving the behavior and
expression unchanged.
In `@packages/apps/base-runtime/src/lib/accessors/modify/ModerationModify.ts`:
- Around line 7-9: Remove the newly added implementation comments from
ModerationModify.ts lines 7-9, SchedulerModify.ts lines 7-9, and UIController.ts
lines 26-28; preserve the surrounding implementation unchanged, with the
rationale retained only in the accessor-consolidation documentation.
In `@packages/apps/base-runtime/src/lib/accessors/read/UserRead.ts`:
- Around line 25-27: Remove the sentinel-behavior implementation comments at
UserRead.ts lines 25-27 and the redundant validateSort comment at RoomRead.ts
line 102; make no other code changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b5d88bec-82c3-402f-8e34-80d4de690c07
📒 Files selected for processing (50)
docs/proposals/apps-accessor-consolidation/README.mdpackages/apps/base-runtime/src/lib/accessors/Persistence.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentRead.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentWrite.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentalVariableRead.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingRead.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingsModify.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingRead.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/environment/tests/environment.test.tspackages/apps/base-runtime/src/lib/accessors/mod.tspackages/apps/base-runtime/src/lib/accessors/modify/ContactCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/EmailCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/MessageUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/ModerationModify.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyDeleter.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/OAuthAppsModify.tspackages/apps/base-runtime/src/lib/accessors/modify/SchedulerModify.tspackages/apps/base-runtime/src/lib/accessors/modify/UIController.tspackages/apps/base-runtime/src/lib/accessors/modify/UploadCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/UserUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/tests/modifyAccessors.test.tspackages/apps/base-runtime/src/lib/accessors/read/CloudWorkspaceRead.tspackages/apps/base-runtime/src/lib/accessors/read/ContactRead.tspackages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.tspackages/apps/base-runtime/src/lib/accessors/read/LivechatRead.tspackages/apps/base-runtime/src/lib/accessors/read/MessageRead.tspackages/apps/base-runtime/src/lib/accessors/read/OAuthAppsReader.tspackages/apps/base-runtime/src/lib/accessors/read/PersistenceRead.tspackages/apps/base-runtime/src/lib/accessors/read/Reader.tspackages/apps/base-runtime/src/lib/accessors/read/RoleRead.tspackages/apps/base-runtime/src/lib/accessors/read/RoomRead.tspackages/apps/base-runtime/src/lib/accessors/read/ThreadRead.tspackages/apps/base-runtime/src/lib/accessors/read/UploadRead.tspackages/apps/base-runtime/src/lib/accessors/read/UserRead.tspackages/apps/base-runtime/src/lib/accessors/read/VideoConferenceRead.tspackages/apps/base-runtime/src/lib/accessors/read/tests/readers.test.tspackages/apps/base-runtime/src/lib/accessors/tests/AppAccessors.test.tspackages/apps/base-runtime/src/lib/accessors/tests/ModifyCreator.test.tspackages/apps/base-runtime/src/lib/accessors/tests/ModifyUpdater.test.tspackages/apps/base-runtime/src/lib/accessors/tests/Persistence.test.tspackages/apps/base-runtime/src/lib/accessors/tests/configuration.test.tspackages/apps/base-runtime/src/lib/bridges/RemoteBridges.tspackages/apps/src/server/bridges/AppResourceBridge.tspackages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/apps/base-runtime/src/lib/accessors/read/CloudWorkspaceRead.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentWrite.tspackages/apps/base-runtime/src/lib/accessors/modify/EmailCreator.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentalVariableRead.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/MessageUpdater.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/read/PersistenceRead.tspackages/apps/base-runtime/src/lib/accessors/tests/Persistence.test.tspackages/apps/base-runtime/src/lib/accessors/modify/OAuthAppsModify.tspackages/apps/base-runtime/src/lib/accessors/modify/UploadCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/ContactCreator.tspackages/apps/base-runtime/src/lib/accessors/read/ThreadRead.tspackages/apps/base-runtime/src/lib/accessors/read/UploadRead.tspackages/apps/base-runtime/src/lib/accessors/read/VideoConferenceRead.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatCreator.tspackages/apps/base-runtime/src/lib/bridges/RemoteBridges.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyDeleter.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingRead.tspackages/apps/base-runtime/src/lib/accessors/read/MessageRead.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentRead.tspackages/apps/base-runtime/src/lib/accessors/read/ContactRead.tspackages/apps/base-runtime/src/lib/accessors/Persistence.tspackages/apps/base-runtime/src/lib/accessors/modify/ModerationModify.tspackages/apps/base-runtime/src/lib/accessors/modify/SchedulerModify.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingRead.tspackages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.tspackages/apps/base-runtime/src/lib/accessors/read/RoleRead.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatUpdater.tspackages/apps/base-runtime/src/lib/accessors/read/OAuthAppsReader.tspackages/apps/base-runtime/src/lib/accessors/tests/ModifyUpdater.test.tspackages/apps/base-runtime/src/lib/accessors/environment/tests/environment.test.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyCreator.tspackages/apps/base-runtime/src/lib/accessors/read/Reader.tspackages/apps/base-runtime/src/lib/accessors/read/tests/readers.test.tspackages/apps/base-runtime/src/lib/accessors/tests/configuration.test.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingsModify.tspackages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.tspackages/apps/base-runtime/src/lib/accessors/tests/ModifyCreator.test.tspackages/apps/base-runtime/src/lib/accessors/modify/UIController.tspackages/apps/base-runtime/src/lib/accessors/modify/UserUpdater.tspackages/apps/base-runtime/src/lib/accessors/read/LivechatRead.tspackages/apps/base-runtime/src/lib/accessors/tests/AppAccessors.test.tspackages/apps/base-runtime/src/lib/accessors/read/UserRead.tspackages/apps/base-runtime/src/lib/accessors/modify/tests/modifyAccessors.test.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.tspackages/apps/src/server/bridges/AppResourceBridge.tspackages/apps/base-runtime/src/lib/accessors/mod.tspackages/apps/base-runtime/src/lib/accessors/read/RoomRead.ts
🧠 Learnings (4)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/apps/base-runtime/src/lib/accessors/read/CloudWorkspaceRead.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentWrite.tspackages/apps/base-runtime/src/lib/accessors/modify/EmailCreator.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentalVariableRead.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/MessageUpdater.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/read/PersistenceRead.tspackages/apps/base-runtime/src/lib/accessors/tests/Persistence.test.tspackages/apps/base-runtime/src/lib/accessors/modify/OAuthAppsModify.tspackages/apps/base-runtime/src/lib/accessors/modify/UploadCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/ContactCreator.tspackages/apps/base-runtime/src/lib/accessors/read/ThreadRead.tspackages/apps/base-runtime/src/lib/accessors/read/UploadRead.tspackages/apps/base-runtime/src/lib/accessors/read/VideoConferenceRead.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatCreator.tspackages/apps/base-runtime/src/lib/bridges/RemoteBridges.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyDeleter.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingRead.tspackages/apps/base-runtime/src/lib/accessors/read/MessageRead.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentRead.tspackages/apps/base-runtime/src/lib/accessors/read/ContactRead.tspackages/apps/base-runtime/src/lib/accessors/Persistence.tspackages/apps/base-runtime/src/lib/accessors/modify/ModerationModify.tspackages/apps/base-runtime/src/lib/accessors/modify/SchedulerModify.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingRead.tspackages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.tspackages/apps/base-runtime/src/lib/accessors/read/RoleRead.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatUpdater.tspackages/apps/base-runtime/src/lib/accessors/read/OAuthAppsReader.tspackages/apps/base-runtime/src/lib/accessors/tests/ModifyUpdater.test.tspackages/apps/base-runtime/src/lib/accessors/environment/tests/environment.test.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyCreator.tspackages/apps/base-runtime/src/lib/accessors/read/Reader.tspackages/apps/base-runtime/src/lib/accessors/read/tests/readers.test.tspackages/apps/base-runtime/src/lib/accessors/tests/configuration.test.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingsModify.tspackages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.tspackages/apps/base-runtime/src/lib/accessors/tests/ModifyCreator.test.tspackages/apps/base-runtime/src/lib/accessors/modify/UIController.tspackages/apps/base-runtime/src/lib/accessors/modify/UserUpdater.tspackages/apps/base-runtime/src/lib/accessors/read/LivechatRead.tspackages/apps/base-runtime/src/lib/accessors/tests/AppAccessors.test.tspackages/apps/base-runtime/src/lib/accessors/read/UserRead.tspackages/apps/base-runtime/src/lib/accessors/modify/tests/modifyAccessors.test.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.tspackages/apps/src/server/bridges/AppResourceBridge.tspackages/apps/base-runtime/src/lib/accessors/mod.tspackages/apps/base-runtime/src/lib/accessors/read/RoomRead.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/apps/base-runtime/src/lib/accessors/read/CloudWorkspaceRead.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentWrite.tspackages/apps/base-runtime/src/lib/accessors/modify/EmailCreator.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentalVariableRead.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/MessageUpdater.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/read/PersistenceRead.tspackages/apps/base-runtime/src/lib/accessors/tests/Persistence.test.tspackages/apps/base-runtime/src/lib/accessors/modify/OAuthAppsModify.tspackages/apps/base-runtime/src/lib/accessors/modify/UploadCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/ContactCreator.tspackages/apps/base-runtime/src/lib/accessors/read/ThreadRead.tspackages/apps/base-runtime/src/lib/accessors/read/UploadRead.tspackages/apps/base-runtime/src/lib/accessors/read/VideoConferenceRead.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatCreator.tspackages/apps/base-runtime/src/lib/bridges/RemoteBridges.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyDeleter.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingRead.tspackages/apps/base-runtime/src/lib/accessors/read/MessageRead.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentRead.tspackages/apps/base-runtime/src/lib/accessors/read/ContactRead.tspackages/apps/base-runtime/src/lib/accessors/Persistence.tspackages/apps/base-runtime/src/lib/accessors/modify/ModerationModify.tspackages/apps/base-runtime/src/lib/accessors/modify/SchedulerModify.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingRead.tspackages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.tspackages/apps/base-runtime/src/lib/accessors/read/RoleRead.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatUpdater.tspackages/apps/base-runtime/src/lib/accessors/read/OAuthAppsReader.tspackages/apps/base-runtime/src/lib/accessors/tests/ModifyUpdater.test.tspackages/apps/base-runtime/src/lib/accessors/environment/tests/environment.test.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyCreator.tspackages/apps/base-runtime/src/lib/accessors/read/Reader.tspackages/apps/base-runtime/src/lib/accessors/read/tests/readers.test.tspackages/apps/base-runtime/src/lib/accessors/tests/configuration.test.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingsModify.tspackages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.tspackages/apps/base-runtime/src/lib/accessors/tests/ModifyCreator.test.tspackages/apps/base-runtime/src/lib/accessors/modify/UIController.tspackages/apps/base-runtime/src/lib/accessors/modify/UserUpdater.tspackages/apps/base-runtime/src/lib/accessors/read/LivechatRead.tspackages/apps/base-runtime/src/lib/accessors/tests/AppAccessors.test.tspackages/apps/base-runtime/src/lib/accessors/read/UserRead.tspackages/apps/base-runtime/src/lib/accessors/modify/tests/modifyAccessors.test.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.tspackages/apps/src/server/bridges/AppResourceBridge.tspackages/apps/base-runtime/src/lib/accessors/mod.tspackages/apps/base-runtime/src/lib/accessors/read/RoomRead.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
packages/apps/base-runtime/src/lib/accessors/read/CloudWorkspaceRead.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentWrite.tspackages/apps/base-runtime/src/lib/accessors/modify/EmailCreator.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentalVariableRead.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/MessageUpdater.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/read/PersistenceRead.tspackages/apps/base-runtime/src/lib/accessors/tests/Persistence.test.tspackages/apps/base-runtime/src/lib/accessors/modify/OAuthAppsModify.tspackages/apps/base-runtime/src/lib/accessors/modify/UploadCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/ContactCreator.tspackages/apps/base-runtime/src/lib/accessors/read/ThreadRead.tspackages/apps/base-runtime/src/lib/accessors/read/UploadRead.tspackages/apps/base-runtime/src/lib/accessors/read/VideoConferenceRead.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatCreator.tspackages/apps/base-runtime/src/lib/bridges/RemoteBridges.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyDeleter.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingRead.tspackages/apps/base-runtime/src/lib/accessors/read/MessageRead.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentRead.tspackages/apps/base-runtime/src/lib/accessors/read/ContactRead.tspackages/apps/base-runtime/src/lib/accessors/Persistence.tspackages/apps/base-runtime/src/lib/accessors/modify/ModerationModify.tspackages/apps/base-runtime/src/lib/accessors/modify/SchedulerModify.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingRead.tspackages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.tspackages/apps/base-runtime/src/lib/accessors/read/RoleRead.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatUpdater.tspackages/apps/base-runtime/src/lib/accessors/read/OAuthAppsReader.tspackages/apps/base-runtime/src/lib/accessors/tests/ModifyUpdater.test.tspackages/apps/base-runtime/src/lib/accessors/environment/tests/environment.test.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyCreator.tspackages/apps/base-runtime/src/lib/accessors/read/Reader.tspackages/apps/base-runtime/src/lib/accessors/read/tests/readers.test.tspackages/apps/base-runtime/src/lib/accessors/tests/configuration.test.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingsModify.tspackages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.tspackages/apps/base-runtime/src/lib/accessors/tests/ModifyCreator.test.tspackages/apps/base-runtime/src/lib/accessors/modify/UIController.tspackages/apps/base-runtime/src/lib/accessors/modify/UserUpdater.tspackages/apps/base-runtime/src/lib/accessors/read/LivechatRead.tspackages/apps/base-runtime/src/lib/accessors/tests/AppAccessors.test.tspackages/apps/base-runtime/src/lib/accessors/read/UserRead.tspackages/apps/base-runtime/src/lib/accessors/modify/tests/modifyAccessors.test.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.tspackages/apps/src/server/bridges/AppResourceBridge.tspackages/apps/base-runtime/src/lib/accessors/mod.tspackages/apps/base-runtime/src/lib/accessors/read/RoomRead.ts
📚 Learning: 2026-05-11T21:46:23.471Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40463
File: packages/apps/src/lib/SecureFields.ts:17-19
Timestamp: 2026-05-11T21:46:23.471Z
Learning: In Rocket.Chat’s `packages/apps/tsconfig.json`, TypeScript `"strict"` is set to `false`, which disables strict type-checking (including `noImplicitAny`) for `packages/apps`. When reviewing, do not flag TS7053 (and similar strict-mode indexing/type errors) in files under `packages/apps/src/` that are a consequence of this relaxed strictness—e.g., patterns like indexing an `unknown`/`object` via optional chaining such as `object?.[kSecureFields]`.
Applied to files:
packages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.tspackages/apps/src/server/bridges/AppResourceBridge.ts
🪛 ast-grep (0.45.0)
packages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import * as child_process from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import * as child_process from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import * as child_process from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import * as child_process from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🪛 LanguageTool
docs/proposals/apps-accessor-consolidation/README.md
[style] ~567-~567: Consider an alternative for the overused word “exactly”.
Context: ...ations coexist transiently, which is exactly what the §6 parity harness guards again...
(EXACTLY_PRECISELY)
🔇 Additional comments (18)
packages/apps/base-runtime/src/lib/accessors/read/CloudWorkspaceRead.ts (1)
1-12: LGTM!packages/apps/base-runtime/src/lib/accessors/environment/EnvironmentalVariableRead.ts (1)
1-19: LGTM!packages/apps/base-runtime/src/lib/accessors/environment/ServerSettingRead.ts (1)
1-15: LGTM!Also applies to: 18-36
packages/apps/base-runtime/src/lib/accessors/environment/ServerSettingUpdater.ts (1)
1-16: LGTM!packages/apps/base-runtime/src/lib/accessors/environment/ServerSettingsModify.ts (1)
1-24: LGTM!packages/apps/base-runtime/src/lib/accessors/environment/SettingRead.ts (1)
1-5: LGTM!Also applies to: 8-17, 20-30
packages/apps/base-runtime/src/lib/accessors/read/PersistenceRead.ts (1)
1-20: LGTM!packages/apps/base-runtime/src/lib/accessors/environment/tests/environment.test.ts (1)
1-88: LGTM!packages/apps/base-runtime/src/lib/accessors/modify/ContactCreator.ts (1)
1-22: LGTM!packages/apps/base-runtime/src/lib/accessors/modify/EmailCreator.ts (1)
1-12: LGTM!packages/apps/base-runtime/src/lib/bridges/RemoteBridges.ts (1)
163-165: LGTM!packages/apps/base-runtime/src/lib/accessors/read/Reader.ts (1)
1-98: LGTM!packages/apps/base-runtime/src/lib/accessors/modify/LivechatUpdater.ts (1)
1-33: LGTM!packages/apps/base-runtime/src/lib/accessors/modify/MessageUpdater.ts (1)
1-16: LGTM!packages/apps/base-runtime/src/lib/accessors/modify/ModifyDeleter.ts (1)
1-38: LGTM!packages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.ts (1)
13-15: LGTM!Also applies to: 36-38
packages/apps/base-runtime/src/lib/accessors/tests/ModifyUpdater.test.ts (1)
107-148: LGTM!packages/apps/base-runtime/src/lib/accessors/tests/configuration.test.ts (1)
1-123: LGTM!
![hacktron-app[bot]](https://avatars.githubusercontent.com/in/1250513?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 50 files
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
d-gubert
force-pushed
the
chore/apps-accessor-consolidation-2
branch
from
July 29, 2026 16:36
c32bff8 to
624af84
Compare
…Environment into base-runtime Moves the read-only and server-settings accessor behavior from the host (src/server/accessors) into the base-runtime as local classes, eliminating the accessor:* JSON-RPC round-trips for these paths - they now emit bridges:* directly with the 'APP_ID' sentinel and are permission-checked host-side by the bridge do* wrappers exactly as before. Ported (accessors/read/*, accessors/environment/*, accessors/Persistence.ts): - Reader family: MessageRead, RoomRead, UserRead, PersistenceRead, LivechatRead, UploadRead, CloudWorkspaceRead, VideoConferenceRead, OAuthAppsReader, ContactRead, ThreadRead, RoleRead, ExperimentalRead, and the Reader facade. - Environment (server-side): EnvironmentRead/EnvironmentWrite facades, ServerSettingRead, EnvironmentalVariableRead, ServerSettingUpdater, ServerSettingsModify. - Persistence. All portable logic moved verbatim: getMessages/getAllRooms/getUnreadByUser validation and option defaults, sort-field validation, association-array wrapping, upsert defaults, getValueById packageValue fallback, and the getAppUser argument-appId exception (kept raw, not normalized). mod.ts flips getReader, getPersistence, the server-settings/env-var members of getEnvironmentRead/getEnvironmentWrite, and getConfigurationModify's serverSettings to the local implementations. App-settings members (getSettings) stay proxied until Phase 3. RPC-boundary adaptation: ServerSettingRead.getValueById now treats null and undefined alike as "not found" (an absent host return serializes to null across the boundary); behaviorally identical for apps. Host accessor classes and AppAccessorManager pruning are intentionally left for the Phase 4 teardown - getReader() is still constructed on the host for AppListenerManager - so this step stays small and green. The parity harness pins the runtime port's emitted bridge traffic against the documented host bridge calls. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018FbxFGJWHiroxrdNPJRL3P
Completes moving the Modify accessor surface into the subprocess runtime, so getReader/getModifier/getPersistence/getHttp now emit zero accessor:* traffic - everything flows through bridges:* with the 'APP_ID' sentinel and stays permission-checked by the host bridge do* wrappers. Ported (accessors/modify/*): - Sub-creators: LivechatCreator (local createToken), UploadCreator (default-user fetch + IUploadDetails assembly), EmailCreator, ContactCreator. - Sub-updaters: MessageUpdater, LivechatUpdater (>0 -> boolean coercion), UserUpdater (partial-object wrapping). - ModifyDeleter (<=50 members cap; deleteUsers argument-appId kept raw), UIController (local UIHelper.assignIds + UIKit interaction formatting via doNotifyUser), SchedulerModify (local createProcessorId id-namespacing), OAuthAppsModify, ModerationModify (argument-appId kept raw). mod.ts getModifier drops the getDeleter/getUiController/getScheduler/ getOAuthAppsModifier/getModerationModifier proxies for local instances, and the runtime ModifyCreator/ModifyUpdater replace their accessor:* sub-creator and sub-updater proxies with the local classes. Identity handling per docs/base-runtime-app-id-exceptions.md: caller-identity params use 'APP_ID'; ModerationModify and ModifyDeleter.deleteUsers forward the app-supplied appId argument raw; UIController and SchedulerModify read the real id from AppObjectRegistry for non-identity stamping (block/interaction appId, job-id suffix). void-returning methods await instead of returning the Promise<unknown> bridge value. Host accessor classes and AppAccessorManager pruning remain for the Phase 4 teardown; the parity harness pins the ported bridge traffic. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018FbxFGJWHiroxrdNPJRL3P
…ResourceBridge Moves the last accessor category off the accessor:* channel. After this phase the base-runtime emits zero accessor:* messages - every accessor call is a bridges:* message, and handleAccessorMessage is dead (removed in Phase 4). Host: - Add AppResourceBridge, a concrete engine-owned bridge (not part of the app-facing AppBridges surface) that delegates to the manager registries (slash commands, APIs, scheduler, UI action buttons, external components, video-conf and outbound providers) and to the app's ProxiedApp storage item + AppSettingsManager for app settings. - BaseRuntimeSubprocessController.handleBridgeMessage resolves the getAppResourceBridge name via a dedicated controller field and suppresses AppResourceBridge.REGISTRATION_METHODS while the subprocess is restarting (replacing the old getConfigurationExtend hijack in handleAccessorMessage, now keyed on an explicit method set). AppManager is handed to the bridge in the controller constructor; no apps/meteor changes required. Permission and conflict semantics are preserved by delegating to the same managers. Runtime: - getConfigurationExtend / getConfigurationModify and the app-settings SettingRead/SettingUpdater/SettingsExtend members now call getAppResourceBridge().do*, keeping the AppObjectRegistry stash-then-forward for slash commands, processors, api endpoints and providers. accessor:api: listApis is replaced by doListApis. registerButton stays a synchronous void. - Removed the now-dead proxify machinery and WithProxy type from mod.ts. RPC-boundary: SettingRead.getValueById treats null/undefined alike as "does not exist" (undefined serializes to null across the boundary). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018FbxFGJWHiroxrdNPJRL3P
Apply the review comments that hold up against the ported host behavior: - Persistence.test: cover the four previously-untested public methods (createWithAssociations, updateByAssociations, remove, removeByAssociations) so their bridge-message format is regression-guarded. - ModifyCreator.test: resolve the app-user lookup to a real user and assert the derived userId is propagated, instead of asserting `undefined` which masked whether default-uploader behavior works. - modifyAccessors.test: exercise deleteMessage, which the test title claimed to cover but never called. - environment.test: add the `value: undefined` case for the getValueById package-value fallback (previously only `null` was exercised). - ModifyDeleter: fix the app-id-exceptions doc path in the explanatory comment to the actual location. Comments that would diverge the runtime copy from the faithfully-ported host accessor (includes-vs-endsWith, getMessages limit/mutation, getOneById null guard, null-vs-undefined return types) are intentionally left as-is. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rm5iQCMuKRqFKExiueC8Jw
Record the SchedulerModify job-id namespacing bug (includes vs endsWith, raised by CodeRabbit + cubic) in §7 follow-ups, with the rationale for deferring it out of the mechanical consolidation: the runtime port is a faithful copy of the host SchedulerModify, so fixing only the runtime copy re-introduces host<->runtime drift. Fix both copies together, or after the host copy is deleted in Phase 4. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rm5iQCMuKRqFKExiueC8Jw
…w-ups Record the remaining automated-review findings (CodeRabbit, cubic) on the ported runtime accessors in §7 as follow-up #7. Each is byte-identical to its host original, so fixing only the runtime copy re-introduces host<->runtime drift; defer to a both-copies change or post Phase-4 teardown. Split into latent-behavior items (uploadBuffer user null-vs-hasOwn, getMessages limit lower bound, getMessages options mutation, getOneById null guard, UIController deprecated block-id scoping) and type/cosmetic items (ContactRead null-vs- undefined, MessageRead/ThreadRead optional return types). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rm5iQCMuKRqFKExiueC8Jw
ggazzo
force-pushed
the
chore/apps-accessor-consolidation-2
branch
from
July 29, 2026 18:35
624af84 to
94bca38
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts (1)
538-540: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the new implementation comments.
The repository guideline for TypeScript/JavaScript says to avoid comments in implementation code. This rationale is already documented in the proposal; keep the controller block comment-free.
As per coding guidelines, TypeScript/JavaScript implementation code should avoid comments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts` around lines 538 - 540, Remove the newly added implementation comment above the AppResourceBridge handling in BaseRuntimeSubprocessController, leaving the surrounding controller logic unchanged and comment-free.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts`:
- Around line 538-540: Remove the newly added implementation comment above the
AppResourceBridge handling in BaseRuntimeSubprocessController, leaving the
surrounding controller logic unchanged and comment-free.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0eebcd28-ba5e-4462-920e-74ce2391ed70
📒 Files selected for processing (50)
docs/proposals/apps-accessor-consolidation/README.mdpackages/apps/base-runtime/src/lib/accessors/Persistence.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentRead.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentWrite.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentalVariableRead.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingRead.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingsModify.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingRead.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/environment/tests/environment.test.tspackages/apps/base-runtime/src/lib/accessors/mod.tspackages/apps/base-runtime/src/lib/accessors/modify/ContactCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/EmailCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/MessageUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/ModerationModify.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyDeleter.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/OAuthAppsModify.tspackages/apps/base-runtime/src/lib/accessors/modify/SchedulerModify.tspackages/apps/base-runtime/src/lib/accessors/modify/UIController.tspackages/apps/base-runtime/src/lib/accessors/modify/UploadCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/UserUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/tests/modifyAccessors.test.tspackages/apps/base-runtime/src/lib/accessors/read/CloudWorkspaceRead.tspackages/apps/base-runtime/src/lib/accessors/read/ContactRead.tspackages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.tspackages/apps/base-runtime/src/lib/accessors/read/LivechatRead.tspackages/apps/base-runtime/src/lib/accessors/read/MessageRead.tspackages/apps/base-runtime/src/lib/accessors/read/OAuthAppsReader.tspackages/apps/base-runtime/src/lib/accessors/read/PersistenceRead.tspackages/apps/base-runtime/src/lib/accessors/read/Reader.tspackages/apps/base-runtime/src/lib/accessors/read/RoleRead.tspackages/apps/base-runtime/src/lib/accessors/read/RoomRead.tspackages/apps/base-runtime/src/lib/accessors/read/ThreadRead.tspackages/apps/base-runtime/src/lib/accessors/read/UploadRead.tspackages/apps/base-runtime/src/lib/accessors/read/UserRead.tspackages/apps/base-runtime/src/lib/accessors/read/VideoConferenceRead.tspackages/apps/base-runtime/src/lib/accessors/read/tests/readers.test.tspackages/apps/base-runtime/src/lib/accessors/tests/AppAccessors.test.tspackages/apps/base-runtime/src/lib/accessors/tests/ModifyCreator.test.tspackages/apps/base-runtime/src/lib/accessors/tests/ModifyUpdater.test.tspackages/apps/base-runtime/src/lib/accessors/tests/Persistence.test.tspackages/apps/base-runtime/src/lib/accessors/tests/configuration.test.tspackages/apps/base-runtime/src/lib/bridges/RemoteBridges.tspackages/apps/src/server/bridges/AppResourceBridge.tspackages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts
🚧 Files skipped from review as they are similar to previous changes (46)
- packages/apps/base-runtime/src/lib/accessors/read/CloudWorkspaceRead.ts
- packages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.ts
- packages/apps/base-runtime/src/lib/accessors/modify/EmailCreator.ts
- packages/apps/base-runtime/src/lib/accessors/modify/ContactCreator.ts
- packages/apps/base-runtime/src/lib/accessors/environment/EnvironmentRead.ts
- packages/apps/base-runtime/src/lib/accessors/modify/OAuthAppsModify.ts
- packages/apps/base-runtime/src/lib/accessors/read/VideoConferenceRead.ts
- packages/apps/base-runtime/src/lib/accessors/environment/EnvironmentWrite.ts
- packages/apps/base-runtime/src/lib/accessors/environment/EnvironmentalVariableRead.ts
- packages/apps/base-runtime/src/lib/accessors/environment/ServerSettingUpdater.ts
- packages/apps/base-runtime/src/lib/accessors/read/OAuthAppsReader.ts
- packages/apps/base-runtime/src/lib/bridges/RemoteBridges.ts
- packages/apps/base-runtime/src/lib/accessors/modify/LivechatCreator.ts
- packages/apps/base-runtime/src/lib/accessors/read/ThreadRead.ts
- packages/apps/base-runtime/src/lib/accessors/modify/MessageUpdater.ts
- packages/apps/base-runtime/src/lib/accessors/environment/SettingRead.ts
- packages/apps/base-runtime/src/lib/accessors/modify/ModifyDeleter.ts
- packages/apps/base-runtime/src/lib/accessors/modify/UploadCreator.ts
- packages/apps/base-runtime/src/lib/accessors/read/PersistenceRead.ts
- packages/apps/base-runtime/src/lib/accessors/read/ContactRead.ts
- packages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.ts
- packages/apps/base-runtime/src/lib/accessors/environment/ServerSettingRead.ts
- packages/apps/base-runtime/src/lib/accessors/modify/UserUpdater.ts
- packages/apps/base-runtime/src/lib/accessors/environment/tests/environment.test.ts
- packages/apps/base-runtime/src/lib/accessors/environment/ServerSettingsModify.ts
- packages/apps/base-runtime/src/lib/accessors/read/UserRead.ts
- packages/apps/base-runtime/src/lib/accessors/modify/ModerationModify.ts
- packages/apps/base-runtime/src/lib/accessors/tests/Persistence.test.ts
- packages/apps/base-runtime/src/lib/accessors/modify/LivechatUpdater.ts
- packages/apps/base-runtime/src/lib/accessors/read/UploadRead.ts
- packages/apps/base-runtime/src/lib/accessors/Persistence.ts
- packages/apps/base-runtime/src/lib/accessors/read/Reader.ts
- packages/apps/base-runtime/src/lib/accessors/read/MessageRead.ts
- packages/apps/base-runtime/src/lib/accessors/modify/tests/modifyAccessors.test.ts
- packages/apps/base-runtime/src/lib/accessors/modify/SchedulerModify.ts
- packages/apps/base-runtime/src/lib/accessors/modify/UIController.ts
- packages/apps/base-runtime/src/lib/accessors/read/RoomRead.ts
- packages/apps/base-runtime/src/lib/accessors/read/RoleRead.ts
- packages/apps/base-runtime/src/lib/accessors/read/tests/readers.test.ts
- packages/apps/base-runtime/src/lib/accessors/tests/configuration.test.ts
- packages/apps/base-runtime/src/lib/accessors/tests/AppAccessors.test.ts
- packages/apps/base-runtime/src/lib/accessors/mod.ts
- packages/apps/base-runtime/src/lib/accessors/modify/ModifyCreator.ts
- packages/apps/base-runtime/src/lib/accessors/read/LivechatRead.ts
- packages/apps/base-runtime/src/lib/accessors/tests/ModifyCreator.test.ts
- packages/apps/src/server/bridges/AppResourceBridge.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
⚠️ CI failures not shown inline (4)
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/apps/base-runtime/src/lib/accessors/tests/ModifyUpdater.test.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.tspackages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts
🧠 Learnings (4)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/apps/base-runtime/src/lib/accessors/tests/ModifyUpdater.test.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.tspackages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/apps/base-runtime/src/lib/accessors/tests/ModifyUpdater.test.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.tspackages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
packages/apps/base-runtime/src/lib/accessors/tests/ModifyUpdater.test.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.tspackages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts
📚 Learning: 2026-05-11T21:46:23.471Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40463
File: packages/apps/src/lib/SecureFields.ts:17-19
Timestamp: 2026-05-11T21:46:23.471Z
Learning: In Rocket.Chat’s `packages/apps/tsconfig.json`, TypeScript `"strict"` is set to `false`, which disables strict type-checking (including `noImplicitAny`) for `packages/apps`. When reviewing, do not flag TS7053 (and similar strict-mode indexing/type errors) in files under `packages/apps/src/` that are a consequence of this relaxed strictness—e.g., patterns like indexing an `unknown`/`object` via optional chaining such as `object?.[kSecureFields]`.
Applied to files:
packages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts
🪛 ast-grep (0.45.0)
packages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import * as child_process from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import * as child_process from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import * as child_process from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import * as child_process from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🪛 LanguageTool
docs/proposals/apps-accessor-consolidation/README.md
[style] ~611-~611: Consider an alternative for the overused word “exactly”.
Context: ...ations coexist transiently, which is exactly what the §6 parity harness guards again...
(EXACTLY_PRECISELY)
🔇 Additional comments (6)
packages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.ts (1)
13-15: LGTM!Also applies to: 36-38
packages/apps/base-runtime/src/lib/accessors/tests/ModifyUpdater.test.ts (1)
107-119: LGTM!Also applies to: 121-134, 136-148
docs/proposals/apps-accessor-consolidation/README.md (1)
496-539: LGTM!Also applies to: 587-618, 619-645, 646-673
packages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts (3)
17-17: LGTM!Also applies to: 126-127, 163-163
532-534: LGTM!Also applies to: 536-537
541-557: LGTM!
d-gubert
force-pushed
the
chore/apps-accessor-consolidation-2
branch
from
July 29, 2026 20:16
94bca38 to
7deb704
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts (1)
538-540: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the implementation comments.
Move this rationale to the architecture documentation and keep the TypeScript implementation comment-free. As per coding guidelines,
*.ts/*.jsimplementation should avoid code comments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts` around lines 538 - 540, Remove the implementation comment describing AppResourceBridge resolution and registration suppression near the restart logic in BaseRuntimeSubprocessController, leaving the surrounding TypeScript behavior unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.ts`:
- Around line 5-7: Remove the implementation comments above the ExperimentalRead
placeholder, including the host/mirror shape and bridges rationale, while
leaving the empty placeholder implementation unchanged.
---
Nitpick comments:
In `@packages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts`:
- Around line 538-540: Remove the implementation comment describing
AppResourceBridge resolution and registration suppression near the restart logic
in BaseRuntimeSubprocessController, leaving the surrounding TypeScript behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dc9bf842-672a-4617-b04b-cea3b7ee547a
📒 Files selected for processing (50)
docs/proposals/apps-accessor-consolidation/README.mdpackages/apps/base-runtime/src/lib/accessors/Persistence.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentRead.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentWrite.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentalVariableRead.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingRead.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/environment/ServerSettingsModify.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingRead.tspackages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.tspackages/apps/base-runtime/src/lib/accessors/environment/tests/environment.test.tspackages/apps/base-runtime/src/lib/accessors/mod.tspackages/apps/base-runtime/src/lib/accessors/modify/ContactCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/EmailCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/LivechatUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/MessageUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/ModerationModify.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyDeleter.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/OAuthAppsModify.tspackages/apps/base-runtime/src/lib/accessors/modify/SchedulerModify.tspackages/apps/base-runtime/src/lib/accessors/modify/UIController.tspackages/apps/base-runtime/src/lib/accessors/modify/UploadCreator.tspackages/apps/base-runtime/src/lib/accessors/modify/UserUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/tests/modifyAccessors.test.tspackages/apps/base-runtime/src/lib/accessors/read/CloudWorkspaceRead.tspackages/apps/base-runtime/src/lib/accessors/read/ContactRead.tspackages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.tspackages/apps/base-runtime/src/lib/accessors/read/LivechatRead.tspackages/apps/base-runtime/src/lib/accessors/read/MessageRead.tspackages/apps/base-runtime/src/lib/accessors/read/OAuthAppsReader.tspackages/apps/base-runtime/src/lib/accessors/read/PersistenceRead.tspackages/apps/base-runtime/src/lib/accessors/read/Reader.tspackages/apps/base-runtime/src/lib/accessors/read/RoleRead.tspackages/apps/base-runtime/src/lib/accessors/read/RoomRead.tspackages/apps/base-runtime/src/lib/accessors/read/ThreadRead.tspackages/apps/base-runtime/src/lib/accessors/read/UploadRead.tspackages/apps/base-runtime/src/lib/accessors/read/UserRead.tspackages/apps/base-runtime/src/lib/accessors/read/VideoConferenceRead.tspackages/apps/base-runtime/src/lib/accessors/read/tests/readers.test.tspackages/apps/base-runtime/src/lib/accessors/tests/AppAccessors.test.tspackages/apps/base-runtime/src/lib/accessors/tests/ModifyCreator.test.tspackages/apps/base-runtime/src/lib/accessors/tests/ModifyUpdater.test.tspackages/apps/base-runtime/src/lib/accessors/tests/Persistence.test.tspackages/apps/base-runtime/src/lib/accessors/tests/configuration.test.tspackages/apps/base-runtime/src/lib/bridges/RemoteBridges.tspackages/apps/src/server/bridges/AppResourceBridge.tspackages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts
🚧 Files skipped from review as they are similar to previous changes (43)
- packages/apps/base-runtime/src/lib/accessors/environment/SettingUpdater.ts
- packages/apps/base-runtime/src/lib/accessors/environment/ServerSettingUpdater.ts
- packages/apps/base-runtime/src/lib/accessors/modify/ContactCreator.ts
- packages/apps/base-runtime/src/lib/accessors/read/CloudWorkspaceRead.ts
- packages/apps/base-runtime/src/lib/accessors/modify/MessageUpdater.ts
- packages/apps/base-runtime/src/lib/accessors/read/PersistenceRead.ts
- packages/apps/base-runtime/src/lib/accessors/read/ThreadRead.ts
- packages/apps/base-runtime/src/lib/accessors/read/VideoConferenceRead.ts
- packages/apps/base-runtime/src/lib/accessors/read/ContactRead.ts
- packages/apps/base-runtime/src/lib/accessors/modify/EmailCreator.ts
- packages/apps/base-runtime/src/lib/accessors/environment/EnvironmentalVariableRead.ts
- packages/apps/base-runtime/src/lib/accessors/environment/ServerSettingsModify.ts
- packages/apps/base-runtime/src/lib/accessors/read/UploadRead.ts
- packages/apps/base-runtime/src/lib/accessors/modify/OAuthAppsModify.ts
- packages/apps/base-runtime/src/lib/accessors/environment/SettingRead.ts
- packages/apps/base-runtime/src/lib/accessors/environment/ServerSettingRead.ts
- packages/apps/base-runtime/src/lib/accessors/read/OAuthAppsReader.ts
- packages/apps/base-runtime/src/lib/accessors/tests/Persistence.test.ts
- packages/apps/base-runtime/src/lib/accessors/modify/LivechatUpdater.ts
- packages/apps/base-runtime/src/lib/accessors/environment/EnvironmentRead.ts
- packages/apps/base-runtime/src/lib/accessors/modify/ModerationModify.ts
- packages/apps/base-runtime/src/lib/accessors/read/Reader.ts
- packages/apps/base-runtime/src/lib/accessors/modify/UploadCreator.ts
- packages/apps/base-runtime/src/lib/accessors/read/RoleRead.ts
- packages/apps/base-runtime/src/lib/accessors/modify/LivechatCreator.ts
- packages/apps/base-runtime/src/lib/bridges/RemoteBridges.ts
- packages/apps/base-runtime/src/lib/accessors/environment/tests/environment.test.ts
- packages/apps/base-runtime/src/lib/accessors/Persistence.ts
- packages/apps/base-runtime/src/lib/accessors/tests/ModifyUpdater.test.ts
- packages/apps/base-runtime/src/lib/accessors/read/MessageRead.ts
- packages/apps/base-runtime/src/lib/accessors/modify/SchedulerModify.ts
- packages/apps/base-runtime/src/lib/accessors/read/tests/readers.test.ts
- packages/apps/base-runtime/src/lib/accessors/modify/ModifyDeleter.ts
- packages/apps/base-runtime/src/lib/accessors/tests/ModifyCreator.test.ts
- packages/apps/base-runtime/src/lib/accessors/modify/tests/modifyAccessors.test.ts
- packages/apps/base-runtime/src/lib/accessors/read/UserRead.ts
- packages/apps/base-runtime/src/lib/accessors/modify/UIController.ts
- packages/apps/base-runtime/src/lib/accessors/modify/UserUpdater.ts
- packages/apps/base-runtime/src/lib/accessors/read/RoomRead.ts
- packages/apps/base-runtime/src/lib/accessors/tests/AppAccessors.test.ts
- packages/apps/base-runtime/src/lib/accessors/mod.ts
- packages/apps/base-runtime/src/lib/accessors/read/LivechatRead.ts
- packages/apps/src/server/bridges/AppResourceBridge.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
⚠️ CI failures not shown inline (4)
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentWrite.tspackages/apps/base-runtime/src/lib/accessors/tests/configuration.test.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyCreator.tspackages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts
🧠 Learnings (5)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentWrite.tspackages/apps/base-runtime/src/lib/accessors/tests/configuration.test.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyCreator.tspackages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentWrite.tspackages/apps/base-runtime/src/lib/accessors/tests/configuration.test.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyCreator.tspackages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
packages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.tspackages/apps/base-runtime/src/lib/accessors/environment/EnvironmentWrite.tspackages/apps/base-runtime/src/lib/accessors/tests/configuration.test.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.tspackages/apps/base-runtime/src/lib/accessors/modify/ModifyCreator.tspackages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts
📚 Learning: 2026-07-29T16:20:05.676Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 41377
File: packages/apps/base-runtime/src/lib/accessors/read/MessageRead.ts:15-20
Timestamp: 2026-07-29T16:20:05.676Z
Learning: During Rocket.Chat’s Apps accessor-consolidation ports, reviewers should verify the Apps accessor read behavior stays consistent with the corresponding existing host accessor for all inputs—including any documented/known edge cases. If a correctness fix is intentionally expected to diverge from the host implementation, it must not be mixed into this port; instead require a dedicated follow-up change/PR that isolates the behavioral difference and clearly documents the reason and expected outcome.
Applied to files:
packages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.ts
📚 Learning: 2026-05-11T21:46:23.471Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40463
File: packages/apps/src/lib/SecureFields.ts:17-19
Timestamp: 2026-05-11T21:46:23.471Z
Learning: In Rocket.Chat’s `packages/apps/tsconfig.json`, TypeScript `"strict"` is set to `false`, which disables strict type-checking (including `noImplicitAny`) for `packages/apps`. When reviewing, do not flag TS7053 (and similar strict-mode indexing/type errors) in files under `packages/apps/src/` that are a consequence of this relaxed strictness—e.g., patterns like indexing an `unknown`/`object` via optional chaining such as `object?.[kSecureFields]`.
Applied to files:
packages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts
🪛 ast-grep (0.45.0)
packages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import * as child_process from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import * as child_process from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import * as child_process from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import * as child_process from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🪛 LanguageTool
docs/proposals/apps-accessor-consolidation/README.md
[style] ~611-~611: Consider an alternative for the overused word “exactly”.
Context: ...ations coexist transiently, which is exactly what the §6 parity harness guards again...
(EXACTLY_PRECISELY)
🔇 Additional comments (6)
packages/apps/base-runtime/src/lib/accessors/modify/ModifyCreator.ts (1)
20-23: LGTM!Also applies to: 48-60
packages/apps/base-runtime/src/lib/accessors/modify/ModifyUpdater.ts (1)
13-15: LGTM!Also applies to: 36-38
packages/apps/base-runtime/src/lib/accessors/tests/configuration.test.ts (1)
1-123: LGTM!packages/apps/base-runtime/src/lib/accessors/environment/EnvironmentWrite.ts (1)
1-16: LGTM!docs/proposals/apps-accessor-consolidation/README.md (1)
496-539: LGTM!Also applies to: 587-618, 619-645, 646-673
packages/apps/src/server/runtime/base/BaseRuntimeSubprocessController.ts (1)
17-17: LGTM!Also applies to: 126-127, 163-163, 532-537, 541-557
Comment on lines
+5
to
+7
| // The host ExperimentalRead is an empty placeholder (no public methods); the runtime | ||
| // mirror keeps the same shape. The bridges reference is retained for parity with the | ||
| // host constructor and for the methods this accessor is expected to grow. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove implementation comments to follow the TypeScript guideline.
These comments are not required for the empty placeholder and violate the instruction to avoid code comments in implementation.
Proposed fix
-// The host ExperimentalRead is an empty placeholder (no public methods); the runtime
-// mirror keeps the same shape. The bridges reference is retained for parity with the
-// host constructor and for the methods this accessor is expected to grow.
export class ExperimentalRead implements IExperimentalRead {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // The host ExperimentalRead is an empty placeholder (no public methods); the runtime | |
| // mirror keeps the same shape. The bridges reference is retained for parity with the | |
| // host constructor and for the methods this accessor is expected to grow. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/apps/base-runtime/src/lib/accessors/read/ExperimentalRead.ts` around
lines 5 - 7, Remove the implementation comments above the ExperimentalRead
placeholder, including the host/mirror shape and bridges rationale, while
leaving the empty placeholder implementation unchanged.
Source: Coding guidelines
ggazzo
approved these changes
Jul 29, 2026
ggazzo
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
This was referenced Jul 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
Issue(s)
Related Jira task: ARCH-2185: reunite duplicated accessor implementations into a shared location
Steps to test or reproduce
Further comments
Summary by CodeRabbit
null/undefinedresults as missing across the RPC boundary.