chore: update deno.lock and freeze install during build - #40038
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:
WalkthroughRead required Deno version from Changes
Sequence Diagram(s)sequenceDiagram
participant Script as Script (deno-cache.js)
participant FS as File System
participant Shell as Shell / CI
participant Deno as Deno binary
Script->>FS: read `.tool-versions`
FS-->>Script: required Deno version string
Script->>Shell: run `deno --version`
Shell-->>Script: installed Deno output or error
Script->>Script: extract versions (regex)
alt deno executable missing (status 127)
Script->>Shell: log "Could not execute deno" with cause / throw
else versions parsed
Script->>Script: compare required vs installed
alt mismatch && CI
Script->>Shell: throw "Incorrect deno version" / abort
else mismatch && !CI
Script->>Shell: console.warn
end
Script->>Deno: run `deno install --entrypoint main.ts` (add `--frozen` if CI)
Deno-->>Script: success or error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 #40038 +/- ##
===========================================
- Coverage 70.55% 70.53% -0.02%
===========================================
Files 3271 3271
Lines 116782 116782
Branches 21090 21041 -49
===========================================
- Hits 82393 82373 -20
- Misses 32338 32359 +21
+ Partials 2051 2050 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Member
Author
|
/layne exception-approve LAYNE-1c1aef2d2ac310a3 LAYNE-139c7c3409afbe19 reason: Neither of the child process calls receive user input at all. Also, they're executed at build time, not run time |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
d-gubert
force-pushed
the
chore/update-deno-lock
branch
from
April 6, 2026 16:06
14eff6f to
e1d1feb
Compare
![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: 1
🧹 Nitpick comments (2)
packages/apps-engine/scripts/deno-cache.js (2)
68-70: Inconsistent error message check and unclear diagnostic logging.The
startsWithcheck uses'Error: Incorrect deno version.'but the actual error message (line 48) starts with'Incorrect Deno version'(different casing, noError:prefix). Additionally, the check result is logged but never used for control flow, making this appear to be leftover debug code.♻️ Proposed cleanup
} else { - console.error(e, e.message, e.message?.startsWith('Error: Incorrect deno version.')); + console.error(e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps-engine/scripts/deno-cache.js` around lines 68 - 70, The console.error block is logging the raw error and a stray boolean check using e.message?.startsWith('Error: Incorrect deno version.') which mismatches the actual message and looks like leftover debug code; update the startsWith comparison to match the real message prefix ("Incorrect Deno version") or remove the boolean check entirely, and replace the noisy console.error(e, e.message, ...) with a clear diagnostic (e.g., console.error("Failed to cache Deno:", e.message) or include e.stack) and, if this is a fatal condition, ensure proper exit handling instead of merely logging; references: the console.error call, e, e.message and the startsWith check.
79-80: Misleading comment: dev mode allows lockfile updates, not prevents them.The comment says "don't update the lockfile" for dev envs, but without
--frozen,deno installwill update the lockfile if dependencies change. Consider clarifying the actual behavior.📝 Suggested comment fix
-// In CI envs, break if lockfile changes; in dev envs, don't update the lockfile +// In CI envs, break if lockfile changes; in dev envs, allow lockfile updates const commandLine = CI ? 'deno install --frozen --entrypoint main.ts' : 'deno install --entrypoint main.ts';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps-engine/scripts/deno-cache.js` around lines 79 - 80, The comment above the commandLine declaration is misleading: CI uses '--frozen' to fail if the lockfile changes, but the dev branch (commandLine when CI is falsy) does not include '--frozen' and therefore may update the lockfile; update the comment to state "in CI envs, break if lockfile changes; in dev envs, allow the lockfile to be updated" (or alternatively add '--frozen' to the non-CI branch if you want to prevent updates), referencing the CI variable and the '--frozen' flag used in the commandLine assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/apps-engine/scripts/deno-cache.js`:
- Around line 47-56: The constructed message string uses the expression '!CI &&
" The server will likely work..."' which inserts the boolean false into message
when CI is truthy; update the message construction in the block comparing
installedVersion and denoToolVersion to use a ternary (e.g. !CI ? " ..." : "")
instead of &&, and also fix the typo "do not to commit" → "do not commit";
ensure you update the local variable message referenced in the if (CI) throw and
console.warn branches.
---
Nitpick comments:
In `@packages/apps-engine/scripts/deno-cache.js`:
- Around line 68-70: The console.error block is logging the raw error and a
stray boolean check using e.message?.startsWith('Error: Incorrect deno
version.') which mismatches the actual message and looks like leftover debug
code; update the startsWith comparison to match the real message prefix
("Incorrect Deno version") or remove the boolean check entirely, and replace the
noisy console.error(e, e.message, ...) with a clear diagnostic (e.g.,
console.error("Failed to cache Deno:", e.message) or include e.stack) and, if
this is a fatal condition, ensure proper exit handling instead of merely
logging; references: the console.error call, e, e.message and the startsWith
check.
- Around line 79-80: The comment above the commandLine declaration is
misleading: CI uses '--frozen' to fail if the lockfile changes, but the dev
branch (commandLine when CI is falsy) does not include '--frozen' and therefore
may update the lockfile; update the comment to state "in CI envs, break if
lockfile changes; in dev envs, allow the lockfile to be updated" (or
alternatively add '--frozen' to the non-CI branch if you want to prevent
updates), referencing the CI variable and the '--frozen' flag used in the
commandLine assignment.
🪄 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
Run ID: 14d77d3d-665f-4e9b-8417-70918f12183c
📒 Files selected for processing (1)
packages/apps-engine/scripts/deno-cache.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- 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-engine/scripts/deno-cache.js
🧠 Learnings (3)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
📚 Learning: 2026-03-18T16:45:52.113Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39701
File: packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts:181-188
Timestamp: 2026-03-18T16:45:52.113Z
Learning: In `packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts`, granting `--allow-read` to the full `this.tempFilePath` (entire temp directory) in the Deno subprocess spawn options is intentional. The `deno.jsonc` config file can affect path resolution in ways that require read access to the broader temp directory, not just the `deno-runtime` subdirectory symlink. Do not flag this as an overly broad permission grant.
Applied to files:
packages/apps-engine/scripts/deno-cache.js
📚 Learning: 2026-03-18T16:47:56.781Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39701
File: packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts:137-151
Timestamp: 2026-03-18T16:47:56.781Z
Learning: In `packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts`, the `EEXIST` catch block in the `DenoRuntimeSubprocessController` constructor (around lines 144–151) intentionally skips validating the target of a pre-existing `deno-runtime` symlink in the temp directory. The rationale is that spoofing the symlink requires the attacker to already have system-level write access to the temp directory, which grants far greater control than the deno-runtime sandbox could provide. Do not flag this as a security issue in future reviews.
Applied to files:
packages/apps-engine/scripts/deno-cache.js
🔇 Additional comments (3)
packages/apps-engine/scripts/deno-cache.js (3)
1-6: LGTM!Constants and imports are correctly set up. Using
127for command-not-found is the standard POSIX convention.
8-29: LGTM!The regex is well-designed with appropriate negative lookbehinds to avoid false positives, and the JSDoc documentation is valuable for understanding this complex pattern.
75-77: LGTM!Path setup is correct and allows
DENO_DIRoverride via environment variable.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/apps-engine/scripts/deno-cache.js (1)
47-52:⚠️ Potential issue | 🟡 MinorTemplate interpolation still appends
falsein CI.Line 48 uses
!CI && "..."inside a template literal, so a CI mismatch throwsIncorrect Deno version...false. It also still leaves thedo not to committypo in that string.In JavaScript, what does `${!process.env.CI && " warning"}` render inside a template literal when `process.env.CI` is set to a non-empty string?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps-engine/scripts/deno-cache.js` around lines 47 - 52, The template literal building message when installedVersion !== denoToolVersion incorrectly uses `${!CI && " ... "}` which inserts "false" in CI and contains the typo "do not to commit"; change that expression to a ternary so it yields an empty string in CI and the full warning otherwise (e.g. `${CI ? "" : " The server will likely work, but it may cause your deno.lock to change - do not commit it. Make sure your Deno version matches the required one so you don't see this message again."}`), updating the text to fix the "do not to commit" typo; update the message variable near installedVersion and denoToolVersion accordingly.
🧹 Nitpick comments (1)
packages/apps-engine/scripts/deno-cache.js (1)
8-29: Prefer explicit parsing over documenting this regex.Lines 8-28 add a long implementation comment just to make the helper readable. Since both inputs have rigid formats, parsing them line-by-line would be simpler to maintain and avoids carrying explanatory comments in the JS implementation. As per coding guidelines, "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps-engine/scripts/deno-cache.js` around lines 8 - 29, The current implementation of extractDenoVersion embeds a complex regex and a long explanatory comment; replace that with explicit, line-by-line parsing to make it simpler and comment-free: remove the multi-line comment and the large single-regex use in extractDenoVersion, split the input into lines, iterate lines and for each line check that it contains the token "deno" (case-insensitive) as a standalone token (e.g., match /\b[Dd]eno\b/ or ensure surrounding characters are not word/dot/dash), then search that same line for a semver-like version using /(\d+\.\d+\.\d+)/ and return the first captured version (or undefined if none); keep the function name extractDenoVersion and its external behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/apps-engine/scripts/deno-cache.js`:
- Around line 79-80: The current commandLine sets '--frozen' only when CI is
true but the comment and intent say dev should not update deno.lock either;
update the assignment for commandLine so the deno install invocation includes
'--frozen' in the non-CI path as well (or remove the CI conditional and always
use '--frozen'), referencing the commandLine variable and the deno install
invocation so the developer ensures deno won't alter deno.lock during local
runs.
---
Duplicate comments:
In `@packages/apps-engine/scripts/deno-cache.js`:
- Around line 47-52: The template literal building message when installedVersion
!== denoToolVersion incorrectly uses `${!CI && " ... "}` which inserts "false"
in CI and contains the typo "do not to commit"; change that expression to a
ternary so it yields an empty string in CI and the full warning otherwise (e.g.
`${CI ? "" : " The server will likely work, but it may cause your deno.lock to
change - do not commit it. Make sure your Deno version matches the required one
so you don't see this message again."}`), updating the text to fix the "do not
to commit" typo; update the message variable near installedVersion and
denoToolVersion accordingly.
---
Nitpick comments:
In `@packages/apps-engine/scripts/deno-cache.js`:
- Around line 8-29: The current implementation of extractDenoVersion embeds a
complex regex and a long explanatory comment; replace that with explicit,
line-by-line parsing to make it simpler and comment-free: remove the multi-line
comment and the large single-regex use in extractDenoVersion, split the input
into lines, iterate lines and for each line check that it contains the token
"deno" (case-insensitive) as a standalone token (e.g., match /\b[Dd]eno\b/ or
ensure surrounding characters are not word/dot/dash), then search that same line
for a semver-like version using /(\d+\.\d+\.\d+)/ and return the first captured
version (or undefined if none); keep the function name extractDenoVersion and
its external 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
Run ID: 25c25ffc-b8e4-46cd-bf3a-1fdcd39c5269
⛔ Files ignored due to path filters (1)
packages/apps-engine/deno-runtime/deno.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
packages/apps-engine/scripts/deno-cache.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: 📦 Build Packages
- 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-engine/scripts/deno-cache.js
🧠 Learnings (4)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39701
File: packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts:137-151
Timestamp: 2026-03-18T16:47:56.781Z
Learning: In `packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts`, the `EEXIST` catch block in the `DenoRuntimeSubprocessController` constructor (around lines 144–151) intentionally skips validating the target of a pre-existing `deno-runtime` symlink in the temp directory. The rationale is that spoofing the symlink requires the attacker to already have system-level write access to the temp directory, which grants far greater control than the deno-runtime sandbox could provide. Do not flag this as a security issue in future reviews.
📚 Learning: 2026-03-18T16:45:52.113Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39701
File: packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts:181-188
Timestamp: 2026-03-18T16:45:52.113Z
Learning: In `packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts`, granting `--allow-read` to the full `this.tempFilePath` (entire temp directory) in the Deno subprocess spawn options is intentional. The `deno.jsonc` config file can affect path resolution in ways that require read access to the broader temp directory, not just the `deno-runtime` subdirectory symlink. Do not flag this as an overly broad permission grant.
Applied to files:
packages/apps-engine/scripts/deno-cache.js
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
packages/apps-engine/scripts/deno-cache.js
📚 Learning: 2026-03-18T16:47:56.781Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39701
File: packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts:137-151
Timestamp: 2026-03-18T16:47:56.781Z
Learning: In `packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts`, the `EEXIST` catch block in the `DenoRuntimeSubprocessController` constructor (around lines 144–151) intentionally skips validating the target of a pre-existing `deno-runtime` symlink in the temp directory. The rationale is that spoofing the symlink requires the attacker to already have system-level write access to the temp directory, which grants far greater control than the deno-runtime sandbox could provide. Do not flag this as a security issue in future reviews.
Applied to files:
packages/apps-engine/scripts/deno-cache.js
d-gubert
force-pushed
the
chore/update-deno-lock
branch
from
April 6, 2026 17:52
2978955 to
70dabb2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/apps-engine/scripts/deno-cache.js (1)
47-56:⚠️ Potential issue | 🟡 MinorBug:
&&operator producesfalsein the output string whenCIis set.When
CIis truthy,!CI && "..."evaluates to the booleanfalse, causing the message to include the literal string"false"instead of nothing. Use a ternary operator instead. Also fix the typo "do not to commit" → "do not commit".🐛 Proposed fix
if (installedVersion !== denoToolVersion) { - const message = `Incorrect Deno version. Required '${denoToolVersion}', found '${installedVersion}'.${!CI && " The server will likely work, but it may cause your deno.lock to change - do not to commit it. Make sure your Deno version matches the required one so you don't see this message again."}`; + const message = `Incorrect Deno version. Required '${denoToolVersion}', found '${installedVersion}'.${!CI ? " The server will likely work, but it may cause your deno.lock to change - do not commit it. Make sure your Deno version matches the required one so you don't see this message again." : ""}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps-engine/scripts/deno-cache.js` around lines 47 - 56, The constructed message uses "!CI && '...'" which evaluates to false when CI is truthy, injecting "false" into the string; update the message assembly in the block that compares installedVersion and denoToolVersion (the variable names installedVersion, denoToolVersion and the CI flag) to use a ternary like CI ? "" : " ..." instead of &&, and also fix the typo "do not to commit" → "do not commit" so the warning reads correctly when CI is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/apps-engine/scripts/deno-cache.js`:
- Around line 68-70: The current check uses e.message?.startsWith('Error:
Incorrect deno version.') which will never match because Error.message lacks the
"Error: " prefix and also logs a stray boolean; update the conditional to detect
a Deno version error by checking e.message (e.g.,
e.message?.startsWith('Incorrect Deno version') or use a regex like /Incorrect
Deno version/i) and remove the boolean from console.error; specifically, replace
the startsWith call on e.message and the extra boolean argument with a proper
message-match (or Error.toString() if you intentionally want the "Error: "
prefix) and call console.error(e) or console.error('Deno version error:',
e.message) as appropriate in the else/then branches.
---
Duplicate comments:
In `@packages/apps-engine/scripts/deno-cache.js`:
- Around line 47-56: The constructed message uses "!CI && '...'" which evaluates
to false when CI is truthy, injecting "false" into the string; update the
message assembly in the block that compares installedVersion and denoToolVersion
(the variable names installedVersion, denoToolVersion and the CI flag) to use a
ternary like CI ? "" : " ..." instead of &&, and also fix the typo "do not to
commit" → "do not commit" so the warning reads correctly when CI is false.
🪄 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
Run ID: 002c3a00-ae9e-4598-8971-d86e29b12e75
📒 Files selected for processing (1)
packages/apps-engine/scripts/deno-cache.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- 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-engine/scripts/deno-cache.js
🧠 Learnings (6)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
📚 Learning: 2026-03-18T16:45:52.113Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39701
File: packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts:181-188
Timestamp: 2026-03-18T16:45:52.113Z
Learning: In `packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts`, granting `--allow-read` to the full `this.tempFilePath` (entire temp directory) in the Deno subprocess spawn options is intentional. The `deno.jsonc` config file can affect path resolution in ways that require read access to the broader temp directory, not just the `deno-runtime` subdirectory symlink. Do not flag this as an overly broad permission grant.
Applied to files:
packages/apps-engine/scripts/deno-cache.js
📚 Learning: 2026-03-18T16:47:56.781Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39701
File: packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts:137-151
Timestamp: 2026-03-18T16:47:56.781Z
Learning: In `packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts`, the `EEXIST` catch block in the `DenoRuntimeSubprocessController` constructor (around lines 144–151) intentionally skips validating the target of a pre-existing `deno-runtime` symlink in the temp directory. The rationale is that spoofing the symlink requires the attacker to already have system-level write access to the temp directory, which grants far greater control than the deno-runtime sandbox could provide. Do not flag this as a security issue in future reviews.
Applied to files:
packages/apps-engine/scripts/deno-cache.js
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
packages/apps-engine/scripts/deno-cache.js
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Applied to files:
packages/apps-engine/scripts/deno-cache.js
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
packages/apps-engine/scripts/deno-cache.js
🔇 Additional comments (3)
packages/apps-engine/scripts/deno-cache.js (3)
1-6: LGTM!The imports and constants are correctly defined. Exit code 127 is the standard POSIX code for "command not found."
8-29: LGTM!The regex is well-documented and correctly extracts the semantic version from both
.tool-versionsformat (deno 2.3.1) anddeno --versionoutput. The named capture group approach is clean.
75-89: LGTM!The implementation correctly applies
--frozenin CI to ensure the lockfile doesn't change, while allowing dev environments to proceed with a warning (from the version check above). This enables developers to update dependencies locally while enforcing immutability in CI.
d-gubert
force-pushed
the
chore/update-deno-lock
branch
from
April 6, 2026 17:54
70dabb2 to
5a2e805
Compare
ggazzo
approved these changes
Apr 9, 2026
sampaiodiego
approved these changes
Apr 9, 2026
Member
|
/layne exception-approve LAYNE-bc53564af2f571b1 LAYNE-d65ee2923b3981c6 reason: Both findings are safe implementation of spawn/exec - one of them is hardcoded and the other one is an envvar. |
|
✅ Exception recorded for LAYNE-bc53564af2f571b1, LAYNE-d65ee2923b3981c6 by @julio-rocketchat: "Both findings are safe implementation of spawn/exec - one of them is hardcoded and the other one is an envvar.". Re-running scan... |
d-gubert
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
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)
With the new upgrade of Deno, building the monorepo with yarn build (which causes the apps-engine to run deno cache to cache deno-runtime dependencies) with a different version during development would cause changes to the deno.lock file - this is not desireable. So we freeze the lock file, breaking the build if changes to it would occur.
Issue(s)
ARCH-2094
Steps to test or reproduce
Further comments
Summary by CodeRabbit