feat(apps): introduce alternative node-runtime#41019
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #41019 +/- ##
===========================================
- Coverage 70.20% 70.15% -0.05%
===========================================
Files 3368 3368
Lines 130022 130022
Branches 22570 22584 +14
===========================================
- Hits 91284 91221 -63
- Misses 35425 35492 +67
+ Partials 3313 3309 -4
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
feat/apps-new-node-runtime
branch
2 times, most recently
from
984de1d to
0fd3c3c
Compare
|
Warning These are security findings reported by the security scanners configured in Layne. Findings may contain false positives - review them and fix what makes sense. Layne found 1 high issue in this PR. View 1 finding(s)
|
|
/layne exception-approve LAYNE-c0ef71515fa6b264 reason: no dynamic surface an attacker could reach to exploit this finding and intended functionality |
|
✅ Exception recorded for LAYNE-c0ef71515fa6b264 by @julio-rocketchat: "no dynamic surface an attacker could reach to exploit this finding and intended functionality". Re-running scan... |
coderabbitai
Bot
added
the
type: feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟡 Minor comments (10)
packages/apps/node-runtime/src/handlers/app/handler.ts-24-25 (1)
24-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
appMethodbefore string operations.
method.split(':')can produceundefinedforappMethodwhen the input format is invalid. Line 59 then calls.startsWith(...), which throws before the intended unknown-method handling path.Proposed fix
const [, appMethod] = method.split(':'); + if (typeof appMethod !== 'string' || appMethod.length === 0) { + throw JsonRpcError.invalidParams({ err: 'Invalid method format' }); + } @@ - } else if (appMethod.startsWith('check') || appMethod.startsWith('execute')) { + } else if (appMethod.startsWith('check') || appMethod.startsWith('execute')) { result = handleListener(request); }Also applies to: 59-61
🤖 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/node-runtime/src/handlers/app/handler.ts` around lines 24 - 25, The issue is that when method.split(':') is executed to extract appMethod, the resulting value can be undefined if the input format is invalid. This undefined value is then used in a .startsWith(...) call around line 59, which throws an error instead of following the intended unknown-method handling path. Add a validation check after the split operation to ensure appMethod is defined and not empty before attempting any string operations on it. If appMethod is invalid, handle it by routing to the unknown-method error handling logic to provide proper error feedback.packages/apps/node-runtime/src/handlers/app/handleUploadEvents.ts-32-40 (1)
32-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
paramsshape before destructuring.Line 39 destructures
paramsimmediately. Invalid/non-array params fall into generic error handling instead of a cleaninvalidParamsresponse.Proposed fix
try { + if (!Array.isArray(params) || !isPlainObject(params[0])) { + throw JsonRpcError.invalidParams({ err: "Invalid 'params' payload" }); + } const [{ file, path }] = params;🤖 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/node-runtime/src/handlers/app/handleUploadEvents.ts` around lines 32 - 40, Before destructuring params on the line with const [{ file, path }] = params;, add validation to ensure params is an array with at least one element. If params fails this validation, return an invalidParams response immediately. This prevents invalid params from falling through to generic error handling and instead provides a clean, specific error response to the caller.packages/apps/node-runtime/src/handlers/app/handleOnInstall.ts-18-23 (1)
18-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
params[0]before invokingonInstall.This currently accepts
[](or non-object first entries) and forwards an invalidcontextinto app code, creating avoidable runtime failures.Suggested fix
- if (!Array.isArray(params)) { + if (!Array.isArray(params) || params.length < 1) { throw new Error('Invalid params', { cause: 'invalid_param_type' }); } - const [context] = params as [Record<string, unknown>]; + const [context] = params; + if (!context || typeof context !== 'object') { + throw new Error('Invalid params', { cause: 'invalid_param_type' }); + }🤖 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/node-runtime/src/handlers/app/handleOnInstall.ts` around lines 18 - 23, The current validation in the handleOnInstall function checks if params is an array but does not validate the contents of that array. Specifically, it does not verify that params is non-empty or that params[0] is a valid object. Add additional validation after the Array.isArray check to ensure params has at least one element and that params[0] is a plain object (typeof check and null check). If either condition fails, throw an Error with appropriate error details. This validation must occur before the destructuring assignment so that invalid context values are never passed to the onInstall function.packages/apps/node-runtime/src/handlers/slashcommand-handler.ts-100-105 (1)
100-105:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate preview context payload before destructuring.
handlePreviewItemchecks onlyparams[0], but destructuresparams[1]immediately after. Missing/invalid second payload currently falls into a runtime destructuring error path.Suggested fix
- if (!Array.isArray(params) || typeof params[0] !== 'object' || !params[0]) { - throw new Error(`First parameter must be an object`); + if ( + !Array.isArray(params) || + typeof params[0] !== 'object' || + !params[0] || + typeof params[1] !== 'object' || + !params[1] + ) { + throw new Error(`Preview item and context parameters must be objects`); }🤖 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/node-runtime/src/handlers/slashcommand-handler.ts` around lines 100 - 105, The validation in the handlePreviewItem function checks that params[0] is a valid object, but does not validate params[1] before attempting to destructure it immediately after. Add validation to ensure params[1] exists and is an object with the required properties (sender, room, params, threadId, triggerId) before performing the destructuring assignment that separates previewItem and the context object containing sender, room, params as args, threadId, and triggerId.packages/apps/node-runtime/src/handlers/outboundcomms-handler.ts-20-29 (1)
20-29:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate provider member is callable before invoking
.apply.Without a function check, invalid method names hit a generic TypeError path and leak runtime internals through
e.message.Suggested fix
const method = provider[methodName as keyof IOutboundMessageProviders]; const { logger } = request.context; const args = (params as Array<unknown>) ?? []; + + if (typeof method !== 'function') { + return new JsonRpcError(`Method ${methodName} not found on outbound communication provider`, -32000); + } try { logger.debug(`Executing ${methodName} on outbound communication provider...`);🤖 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/node-runtime/src/handlers/outboundcomms-handler.ts` around lines 20 - 29, The code attempts to call the apply method on a provider method without validating that it is actually a callable function. This can result in generic TypeErrors that expose runtime internals when an invalid method name is provided. After assigning the method from the provider object using methodName as the key, add a validation check to ensure the method is typeof function before the try block. If the method is not callable, throw a descriptive error that clearly indicates the invalid method name was provided to the outbound communication provider instead of allowing the generic error to propagate.packages/apps/src/server/runtime/node/AppsEngineNodeRuntime.ts-54-58 (1)
54-58:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse non-coercive timeout parsing for
APPS_ENGINE_RUNTIME_TIMEOUT.Line 56 uses global
isFinite(...), where''becomes0. A blank env value can unintentionally set zero timeout and make requests fail immediately.Proposed diff
function getRuntimeTimeout() { const defaultTimeout = 30000; - const envValue = isFinite(process.env.APPS_ENGINE_RUNTIME_TIMEOUT as any) - ? Number(process.env.APPS_ENGINE_RUNTIME_TIMEOUT) - : defaultTimeout; + const parsed = Number(process.env.APPS_ENGINE_RUNTIME_TIMEOUT); + const envValue = Number.isFinite(parsed) ? parsed : defaultTimeout;🤖 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/node/AppsEngineNodeRuntime.ts` around lines 54 - 58, The getRuntimeTimeout function uses global isFinite() on the raw environment variable string, which relies on type coercion and allows empty strings to pass validation (coercing to 0). This can unintentionally set a zero timeout. Replace the isFinite check with non-coercive parsing by first parsing the environment variable to a number and then validating it with Number.isFinite() on the parsed value, or implement an explicit check that validates the string is a non-empty numeric value before calling Number() on it.packages/apps/node-runtime/src/main.ts-21-22 (1)
21-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate subprocess banner text to Node runtime wording.
Lines [21]-[22] still describe this binary as a Deno wrapper, which is now misleading for runtime diagnostics.
Suggested fix
- This is a Deno wrapper for Rocket.Chat Apps. It is not meant to be executed stand-alone; + This is a Node wrapper for Rocket.Chat Apps. It is not meant to be executed stand-alone;🤖 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/node-runtime/src/main.ts` around lines 21 - 22, The banner text in the main.ts file currently references this as a "Deno wrapper" which is outdated for the Node runtime. Update the comment text on lines 21-22 to replace the reference to "Deno wrapper" with appropriate wording that describes this as a Node runtime wrapper, while keeping the context that it is meant to be executed as a subprocess by the Apps-Engine framework.packages/apps/node-runtime/src/lib/ast/tests/data/ast_blocks.ts-104-106 (1)
104-106:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix fixture/code mismatch in
AssignmentExpressionOfNamedFunctionToFooMemberExpression.The fixture says
obj.foo = function bar() {}, but the AST encodes objectaand an anonymous function (id: null). This makes the test data inconsistent with its own scenario name and source string.Suggested patch
export const AssignmentExpressionOfNamedFunctionToFooMemberExpression: TestNodeExcerpt<ExpressionStatement> = { code: 'obj.foo = function bar() {}', node: { @@ left: { type: 'MemberExpression', object: { type: 'Identifier', - name: 'a', + name: 'obj', ...startEnd, }, @@ right: { type: 'FunctionExpression', - id: null, + id: { + type: 'Identifier', + name: 'bar', + ...startEnd, + }, expression: false,Also applies to: 114-116, 128-130
🤖 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/node-runtime/src/lib/ast/tests/data/ast_blocks.ts` around lines 104 - 106, The test fixture AssignmentExpressionOfNamedFunctionToFooMemberExpression and related test fixtures at lines 114-116 and 128-130 have inconsistencies between their code strings and corresponding AST node data. The code string shows obj.foo = function bar() {} but the AST node encodes a different object name (such as a) and an anonymous function (id: null) instead of the named function. Update each fixture's node property to ensure the AST structure matches exactly what the code string represents, including the correct object identifier and function name declarations where applicable.packages/apps/node-runtime/src/lib/accessors/notifier.ts-43-57 (1)
43-57:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid mutating the caller-provided
optionsobject intyping().
typing()currently editsoptionsin place and reuses it in the returned callback. If the caller reuses/mutates that object, stop-typing can send unintended payload values.Suggested patch
public async typing(options: ITypingOptions): Promise<() => Promise<void>> { - options.scope = options.scope || TypingScope.Room; - - if (!options.username) { + let username = options.username; + if (!username) { const appUser = await this.getAppUser(); - options.username = (appUser && appUser.name) || ''; + username = (appUser && appUser.name) || ''; } + const normalizedOptions: ITypingOptions = { + ...options, + scope: options.scope || TypingScope.Room, + username, + }; const appId = AppObjectRegistry.get<string>('id'); - await this.callMessageBridge('doTyping', [{ ...options, isTyping: true }, appId]); + await this.callMessageBridge('doTyping', [{ ...normalizedOptions, isTyping: true }, appId]); return async () => { - await this.callMessageBridge('doTyping', [{ ...options, isTyping: false }, appId]); + await this.callMessageBridge('doTyping', [{ ...normalizedOptions, isTyping: false }, appId]); }; }🤖 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/node-runtime/src/lib/accessors/notifier.ts` around lines 43 - 57, The typing() method currently mutates the caller-provided options object in place by modifying options.scope and options.username, which is then reused in the returned callback. If the caller later mutates the original options object, it can cause unintended values to be sent in the stop-typing call. Create a shallow copy of the options object at the beginning of the typing() method and perform all modifications (setting scope and username) on the copy instead. Use this copied options object consistently in both the initial callMessageBridge call for start typing and in the returned callback for stop typing to ensure the method sends the same payload values regardless of caller mutations.packages/apps/node-runtime/src/lib/accessors/builders/MessageBuilder.ts-243-245 (1)
243-245:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate custom-field detection is incorrect for falsy existing values.
The current check misses existing keys when the stored value is falsy (
false,0,''), allowing unintended overwrite.Proposed fix
- if (this.msg.customFields[key]) { + if (Object.prototype.hasOwnProperty.call(this.msg.customFields, key)) { throw new Error(`The message already contains a custom field by the key: ${key}`); }🤖 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/node-runtime/src/lib/accessors/builders/MessageBuilder.ts` around lines 243 - 245, The duplicate custom-field detection in MessageBuilder is using a truthy check on the value (if this.msg.customFields[key]) which fails to detect existing keys when the stored value is falsy like false, 0, or empty string. Replace this truthy check with a proper key existence check that works regardless of the value type, such as using hasOwnProperty, the in operator, or Object.hasOwn on the customFields object to verify the key exists before throwing the error.
🧹 Nitpick comments (12)
packages/apps/node-runtime/src/handlers/app/handler.ts (1)
27-27: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove inline implementation comment to match TS/JS guideline.
As per coding guidelines, "
**/*.{ts,tsx,js}: 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/node-runtime/src/handlers/app/handler.ts` at line 27, Remove the inline comment that explains the getStatus method handling on line 27 in the handler file, as this violates the TypeScript/JavaScript coding guideline that restricts code comments in implementation. The comment explains why the getStatus method is handled separately to avoid generating logs, but this rationale should be conveyed through clear code structure and naming rather than inline comments.Source: Coding guidelines
packages/apps/node-runtime/src/handlers/app/construct.ts (1)
29-33: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove inline implementation comments in this TS module.
This file has inline implementation comments; the repository guideline for
**/*.{ts,tsx,js}asks to avoid code comments in implementation.As per coding guidelines, "
**/*.{ts,tsx,js}: Avoid code comments in the implementation".Also applies to: 103-105
🤖 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/node-runtime/src/handlers/app/construct.ts` around lines 29 - 33, The inline implementation comments above the buildRequire() function and the comments at lines 103-105 violate the repository coding guideline that requires avoiding code comments in TypeScript implementation files. Remove these inline comments entirely while preserving the actual code implementation. If the comments contain important information that cannot be derived from the code itself, consider refactoring the code to be more self-documenting through clearer variable names or function names instead of relying on comments.Source: Coding guidelines
packages/apps/node-runtime/src/handlers/listener/handler.ts (1)
59-62: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove inline implementation comments to match repository rule.
This module contains multiple explanatory inline comments in implementation code; the project guideline for TS/JS implementation files asks to avoid them.
As per coding guidelines,
**/*.{ts,tsx,js}should “Avoid code comments in the implementation”.Also applies to: 84-87, 94-95, 97-98, 108-109, 119-123, 134-139, 155-156
🤖 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/node-runtime/src/handlers/listener/handler.ts` around lines 59 - 62, Remove all inline implementation comments from the handler module that explain code functionality or parameters. This includes the parameter documentation comment block at lines 59-62 and all other explanatory comments throughout the file at the specified ranges (84-87, 94-95, 97-98, 108-109, 119-123, 134-139, 155-156). Replace these comments with self-documenting code where necessary, or remove them entirely to comply with the project guideline that discourages code comments in TypeScript/JavaScript implementation files.Source: Coding guidelines
packages/apps/node-runtime/src/handlers/scheduler-handler.ts (1)
40-45: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove inline implementation comments in handler body.
These comments violate the repository guideline to avoid code comments in implementation. Prefer extracting this rationale to a short ADR/PR note or a named helper that makes intent self-evident.
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/node-runtime/src/handlers/scheduler-handler.ts` around lines 40 - 45, Remove the multi-line comment block (lines 40-45) that explains the processor registration behavior and Logger binding workaround from the scheduler-handler.ts file. Either document this rationale in an ADR or PR description, or refactor the code to make the intent self-evident through clearer variable names and function structure rather than relying on inline comments to explain the implementation logic.Source: Coding guidelines
packages/apps/node-runtime/src/handlers/tests/videoconference-handler.test.ts (1)
69-83: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a regression test where middle optional arg is omitted.
Please add coverage for
params: ['call', undefined, 'options']and assert the provider receivesundefinedin the second positional slot.🤖 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/node-runtime/src/handlers/tests/videoconference-handler.test.ts` around lines 69 - 83, The test named "correctly handles execution of a videoconf method with three params" currently only covers the case where all three parameters are provided. Add a new test case to cover the regression scenario where the middle optional argument is omitted by passing params as ['call', undefined, 'options'] instead of ['call', 'user', 'options']. In this new test, verify that the mockProvider.three method is called with the correct number of arguments and that the undefined value is properly passed through in the second positional slot, ensuring the handler correctly handles undefined values in the middle of the parameter list.packages/apps/node-runtime/src/handlers/tests/api-handler.test.ts (1)
158-176: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd malformed
paramscoverage for API handler.Please add a test for missing/non-array
paramsso the handler’s-32602path stays protected against regressions.🤖 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/node-runtime/src/handlers/tests/api-handler.test.ts` around lines 158 - 176, Add test coverage for malformed params in the API handler to ensure the error handling path for `-32602` remains protected. Create new test cases using `createMockRequest` that pass `params` as undefined, null, or a non-array value to verify the `apiHandler` function correctly validates the params parameter and returns the appropriate error response. This will ensure the handler's validation logic for the params field doesn't regress in future changes.packages/apps/node-runtime/src/handlers/slashcommand-handler.ts (1)
54-59: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove implementation comments to align with repository rule.
As per coding guidelines, "Avoid code comments in the implementation."
Also applies to: 90-94
🤖 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/node-runtime/src/handlers/slashcommand-handler.ts` around lines 54 - 59, Remove the JSDoc comment blocks from the slashcommand handler that document method parameters to comply with the repository guideline against implementation comments. Delete the entire comment block at lines 54-59 that contains the `@param` documentation for deps, command, method, and params, as well as the similar comment block at lines 90-94. These JSDoc blocks should be removed entirely while keeping the actual method implementation intact.Source: Coding guidelines
packages/apps/package.json (1)
14-18: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winClean
node-runtime/distinbuild:clean.Line 15 only removes
dist. Sincenode-runtime/is shipped (Line 10), stale files innode-runtime/distcan survive and leak into artifacts.Proposed diff
- "build:clean": "rimraf dist", + "build:clean": "rimraf dist node-runtime/dist",🤖 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/package.json` around lines 14 - 18, The build:clean script in package.json only removes the dist directory, but since node-runtime/ is included in the package distribution, stale files in node-runtime/dist can persist and contaminate artifacts. Update the build:clean script by modifying the rimraf command to remove both dist and node-runtime/dist directories to ensure all build artifacts are properly cleaned before each build.packages/apps/node-runtime/src/lib/accessors/http.ts (1)
25-25: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove commented-out implementation code.
As per coding guidelines,
**/*.{ts,tsx,js}: “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/node-runtime/src/lib/accessors/http.ts` at line 25, Remove the commented-out line containing the HttpExtender initialization in the http.ts file. The line `// this.httpExtender = new HttpExtend();` should be deleted entirely to comply with the coding guideline that avoids leaving commented-out implementation code in the codebase.Source: Coding guidelines
packages/apps/node-runtime/src/lib/wrapAppForRequest.ts (1)
18-19: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove inline implementation comments to match repo rule.
As per coding guidelines,
**/*.{ts,tsx,js}: “Avoid code comments in the implementation”.Also applies to: 30-31
🤖 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/node-runtime/src/lib/wrapAppForRequest.ts` around lines 18 - 19, Remove the inline implementation comments at lines 18-19 in the wrapAppForRequest function that explain the caching logic for object instances. Also remove the similar inline implementation comments at lines 30-31 that explain the same caching behavior. These comments explain implementation details and should be removed to comply with the repository's coding guideline that discourages implementation-level comments in TypeScript files.Source: Coding guidelines
packages/apps/node-runtime/src/lib/accessors/mod.ts (1)
37-37: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove inline implementation comments to comply with project style.
As per coding guidelines,
**/*.{ts,tsx,js}: “Avoid code comments in the implementation”.Also applies to: 42-42, 86-87, 91-92, 144-145, 179-180, 185-186, 198-199, 210-210, 232-233
🤖 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/node-runtime/src/lib/accessors/mod.ts` at line 37, Remove all inline implementation comments from the file packages/apps/node-runtime/src/lib/accessors/mod.ts to comply with project coding guidelines that prohibit code comments in implementation. This includes the comment about the _proxy property delegation at line 37, and all other similar comments located at lines 42, 86-87, 91-92, 144-145, 179-180, 185-186, 198-199, 210, and 232-233. Simply delete these comment lines while preserving all functional code.Source: Coding guidelines
packages/apps/node-runtime/src/lib/accessors/builders/BlockBuilder.ts (1)
9-15: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove implementation-level JSDoc in runtime code to match repo rule.
Keep deprecation notes in external docs/types, but avoid inline implementation comments in this file.
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/node-runtime/src/lib/accessors/builders/BlockBuilder.ts` around lines 9 - 15, Remove the implementation-level JSDoc comment from the BlockBuilder class that explains the constructor override and registry sourcing behavior. Keep only the deprecation note if necessary, but prefer moving deprecation information to external documentation rather than inline code comments, as per the repository guideline to avoid code comments in the implementation.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72cb659f-fa9a-42e1-a406-3f3ec1e67bfd
📒 Files selected for processing (93)
.github/workflows/ci-test-e2e.yml.github/workflows/ci.ymlapps/meteor/.mocharc.api.apps.jsapps/meteor/package.jsondocker-compose-ci.ymlpackages/apps/deno-runtime/lib/tests/secureFields.test.tspackages/apps/node-runtime/src/AppObjectRegistry.tspackages/apps/node-runtime/src/error-handlers.tspackages/apps/node-runtime/src/globals.d.tspackages/apps/node-runtime/src/handlers/api-handler.tspackages/apps/node-runtime/src/handlers/app/construct.tspackages/apps/node-runtime/src/handlers/app/handleGetStatus.tspackages/apps/node-runtime/src/handlers/app/handleInitialize.tspackages/apps/node-runtime/src/handlers/app/handleOnDisable.tspackages/apps/node-runtime/src/handlers/app/handleOnEnable.tspackages/apps/node-runtime/src/handlers/app/handleOnInstall.tspackages/apps/node-runtime/src/handlers/app/handleOnPreSettingUpdate.tspackages/apps/node-runtime/src/handlers/app/handleOnSettingUpdated.tspackages/apps/node-runtime/src/handlers/app/handleOnUninstall.tspackages/apps/node-runtime/src/handlers/app/handleOnUpdate.tspackages/apps/node-runtime/src/handlers/app/handleSetStatus.tspackages/apps/node-runtime/src/handlers/app/handleUploadEvents.tspackages/apps/node-runtime/src/handlers/app/handler.tspackages/apps/node-runtime/src/handlers/lib/assertions.tspackages/apps/node-runtime/src/handlers/listener/handler.tspackages/apps/node-runtime/src/handlers/outboundcomms-handler.tspackages/apps/node-runtime/src/handlers/scheduler-handler.tspackages/apps/node-runtime/src/handlers/slashcommand-handler.tspackages/apps/node-runtime/src/handlers/tests/api-handler.test.tspackages/apps/node-runtime/src/handlers/tests/helpers/mod.tspackages/apps/node-runtime/src/handlers/tests/listener-handler.test.tspackages/apps/node-runtime/src/handlers/tests/scheduler-handler.test.tspackages/apps/node-runtime/src/handlers/tests/slashcommand-handler.test.tspackages/apps/node-runtime/src/handlers/tests/uikit-handler.test.tspackages/apps/node-runtime/src/handlers/tests/upload-event-handler.test.tspackages/apps/node-runtime/src/handlers/tests/videoconference-handler.test.tspackages/apps/node-runtime/src/handlers/uikit/handler.tspackages/apps/node-runtime/src/handlers/videoconference-handler.tspackages/apps/node-runtime/src/lib/accessors/builders/BlockBuilder.tspackages/apps/node-runtime/src/lib/accessors/builders/DiscussionBuilder.tspackages/apps/node-runtime/src/lib/accessors/builders/LivechatMessageBuilder.tspackages/apps/node-runtime/src/lib/accessors/builders/MessageBuilder.tspackages/apps/node-runtime/src/lib/accessors/builders/RoomBuilder.tspackages/apps/node-runtime/src/lib/accessors/builders/UserBuilder.tspackages/apps/node-runtime/src/lib/accessors/builders/VideoConferenceBuilder.tspackages/apps/node-runtime/src/lib/accessors/extenders/HttpExtender.tspackages/apps/node-runtime/src/lib/accessors/extenders/MessageExtender.tspackages/apps/node-runtime/src/lib/accessors/extenders/RoomExtender.tspackages/apps/node-runtime/src/lib/accessors/extenders/VideoConferenceExtend.tspackages/apps/node-runtime/src/lib/accessors/formatResponseErrorHandler.tspackages/apps/node-runtime/src/lib/accessors/http.tspackages/apps/node-runtime/src/lib/accessors/mod.tspackages/apps/node-runtime/src/lib/accessors/modify/ModifyCreator.tspackages/apps/node-runtime/src/lib/accessors/modify/ModifyExtender.tspackages/apps/node-runtime/src/lib/accessors/modify/ModifyUpdater.tspackages/apps/node-runtime/src/lib/accessors/notifier.tspackages/apps/node-runtime/src/lib/accessors/tests/AppAccessors.test.tspackages/apps/node-runtime/src/lib/accessors/tests/ModifyCreator.test.tspackages/apps/node-runtime/src/lib/accessors/tests/ModifyExtender.test.tspackages/apps/node-runtime/src/lib/accessors/tests/ModifyUpdater.test.tspackages/apps/node-runtime/src/lib/accessors/tests/formatResponseErrorHandler.test.tspackages/apps/node-runtime/src/lib/accessors/tests/http.test.tspackages/apps/node-runtime/src/lib/ast/acorn-walk.d.tspackages/apps/node-runtime/src/lib/ast/acorn.d.tspackages/apps/node-runtime/src/lib/ast/mod.tspackages/apps/node-runtime/src/lib/ast/operations.tspackages/apps/node-runtime/src/lib/ast/tests/data/ast_blocks.tspackages/apps/node-runtime/src/lib/ast/tests/operations.test.tspackages/apps/node-runtime/src/lib/codec.tspackages/apps/node-runtime/src/lib/loader-hook.tspackages/apps/node-runtime/src/lib/logger.tspackages/apps/node-runtime/src/lib/messenger.tspackages/apps/node-runtime/src/lib/metricsCollector.tspackages/apps/node-runtime/src/lib/parseArgs.tspackages/apps/node-runtime/src/lib/requestContext.tspackages/apps/node-runtime/src/lib/room.tspackages/apps/node-runtime/src/lib/roomFactory.tspackages/apps/node-runtime/src/lib/sanitizeDeprecatedUsage.tspackages/apps/node-runtime/src/lib/secureFields.tspackages/apps/node-runtime/src/lib/tests/logger.test.tspackages/apps/node-runtime/src/lib/tests/messenger.test.tspackages/apps/node-runtime/src/lib/tests/secureFields.test.tspackages/apps/node-runtime/src/lib/wrapAppForRequest.tspackages/apps/node-runtime/src/main.tspackages/apps/node-runtime/tsconfig.jsonpackages/apps/package.jsonpackages/apps/src/server/managers/AppRuntimeManager.tspackages/apps/src/server/runtime/AppsEngineNodeRuntime.tspackages/apps/src/server/runtime/node/AppsEngineNodeRuntime.tspackages/apps/src/server/runtime/node/LivenessManager.tspackages/apps/src/server/runtime/node/ProcessMessenger.tspackages/apps/src/server/runtime/node/bundler.tspackages/apps/src/server/runtime/node/codec.ts
💤 Files with no reviewable changes (1)
- packages/apps/src/server/runtime/AppsEngineNodeRuntime.ts
| private async processQueue() { | ||
| if (this.isProcessing) { | ||
| return; | ||
| } | ||
|
|
||
| this.isProcessing = true; | ||
|
|
||
| while (this.queue.length) { | ||
| const message = this.queue.shift(); | ||
|
|
||
| if (message) { | ||
| await Transport.send(message); | ||
| } | ||
| } | ||
|
|
||
| this.isProcessing = false; | ||
| } |
There was a problem hiding this comment.
Always release queue processing state on transport errors.
Lines [40]-[56] can exit early on Transport.send rejection and leave isProcessing stuck true, stalling the entire outbound channel.
Suggested fix
private async processQueue() {
if (this.isProcessing) {
return;
}
this.isProcessing = true;
- while (this.queue.length) {
- const message = this.queue.shift();
-
- if (message) {
- await Transport.send(message);
- }
- }
-
- this.isProcessing = false;
+ try {
+ while (this.queue.length) {
+ const message = this.queue[0];
+ if (!message) {
+ this.queue.shift();
+ continue;
+ }
+
+ await Transport.send(message);
+ this.queue.shift();
+ }
+ } finally {
+ this.isProcessing = false;
+ }
}🤖 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/node-runtime/src/lib/messenger.ts` around lines 40 - 56, The
processQueue method can exit early if Transport.send rejects an error, leaving
the isProcessing flag stuck at true and blocking all future queue processing.
Wrap the while loop that processes messages in a try-finally block so that the
isProcessing = false statement always executes, regardless of whether
Transport.send throws an error or completes successfully.
| if (Messenger.isRequest(JSONRPCMessage)) { | ||
| void requestRouter(JSONRPCMessage); | ||
| continue; |
There was a problem hiding this comment.
Do not drop requestRouter rejections in the main loop.
Lines [112]-[114] call requestRouter without awaiting/catching. Handler failures bypass this loop’s error handling, producing unhandled rejections and missing RPC responses.
Suggested fix
if (Messenger.isRequest(JSONRPCMessage)) {
- void requestRouter(JSONRPCMessage);
+ await requestRouter(JSONRPCMessage);
continue;
}📝 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.
| if (Messenger.isRequest(JSONRPCMessage)) { | |
| void requestRouter(JSONRPCMessage); | |
| continue; | |
| if (Messenger.isRequest(JSONRPCMessage)) { | |
| await requestRouter(JSONRPCMessage); | |
| continue; | |
| } |
🤖 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/node-runtime/src/main.ts` around lines 112 - 114, The call to
requestRouter with the void operator in the main loop discards any promise
rejections, causing unhandled rejections and missing RPC responses. Modify the
code block where Messenger.isRequest checks the JSONRPCMessage to properly
handle the promise returned by requestRouter by adding appropriate error
handling (either await with try-catch or then-catch) so that any handler
failures are caught and processed by the loop's error handling mechanism instead
of being silently dropped.
17c8548 to
ba2d17d
Compare
d-gubert
force-pushed
the
feat/apps-new-node-runtime
branch
5 times, most recently
from
9d0d1fa to
0bd8c2f
Compare
…o-runtime Remove .ts extensions from relative imports and replace the require(...) helper pattern with direct ESM imports, aligning the deno-runtime sources with node-runtime so the two trees converge toward a shared base runtime. Mechanical, no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Document the plan to unify the near-duplicate deno-runtime and node-runtime trees behind a single base runtime with a thin per-platform adapter (RuntimePlatform interface). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the relocated require(...) helper pattern with direct
ESM 'import { X } from "….js"' statements for apps-engine runtime
classes. Mechanical, no behavior change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
d-gubert
force-pushed
the
feat/apps-new-node-runtime
branch
from
64e09cf to
1a87551
Compare
2 participants
Proposed changes (including videos or screenshots)
Issue(s)
ARCH-2189
Steps to test or reproduce
Further comments
File count is high because we're duplicating the deno-runtime files as node-runtime (with a little adapting to swap usage of Deno API to Node API). The behavior in the node-runtime directory is intended to match what exists in deno-runtime - no architectural refactor has been done
Summary by CodeRabbit
Release Notes
New Features
Testing