fix(release): fail closed on unsafe release tags - #3452
Conversation
yarn release:tag only warned on a version that was not greater than the latest tag in its channel, and never checked that HEAD was the commit actually merged to master — so tagging the pre-merge bump commit on the release branch silently shipped the wrong tree. The prompt also had no non-interactive path, which pushed callers toward hand-rolled git tag commands that skip every guard. - verify HEAD is contained in origin/master before tagging; hard error with both SHAs and remediation, escape hatch --allow-detached-from-master - channel regression is now a hard error, overridable with --force - add --yes/-y (and CI=true) to skip the confirmation prompt - add --help - extract pure tag/channel logic into scripts/releaseTag.lib.ts and cover it with unit tests, under a new node-environment Jest project An existing tag is still never overridable, --force included: the remedy is to bump the version, not to force the tag.
AGENTS.md had nothing on release tagging, so the yarn release:tag guards and their overrides were undiscoverable outside the script itself. - add a Releases And Tagging section covering the guard table, flags, the tag-the-merge-commit rule, and the fresh-worktree node_modules trap - ship-release: use yarn release:tag --yes and drop the manual git tag fallback, which bypassed the guards - ship-release: promote the Jira release sync from an optional wrap-up line to its own phase — resolve the [Electron] X.Y.Z version, set fixVersion on every shipped issue, create issues for work that has none, transition to Done, and verify by JQL rather than trusting the write responses
|
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 release process now uses guarded ChangesRelease flow
CI and test runtime
Desktop release action
UI behavior
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant workflow as ship-release skill
participant tagger as yarn release:tag
participant git as Git origin/master
participant jira as Jira REST and CLI
workflow->>tagger: Run guarded release tagging
tagger->>git: Fetch and verify merge ancestry
git-->>tagger: Return ancestry and tag data
tagger-->>workflow: Create and verify release tag
workflow->>jira: Synchronize release and shipped issues
jira-->>workflow: Return release URL and issue count
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
scripts/releaseTag.lib.ts (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant comment.
normalizeTagand its conditional state this behavior.As per coding guidelines: “Avoid unnecessary comments; prefer self-documenting code through clear naming.”
🤖 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 `@scripts/releaseTag.lib.ts` at line 13, Remove the redundant “Strip leading 'v'...” comment near normalizeTag, leaving the self-documenting normalizeTag implementation and its conditional 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 @.claude/skills/ship-release/SKILL.md:
- Line 190: Update the release documentation sentence to remove the subjective
“far cheaper” comparison and state only the operational outcome: keeping the
information current per release avoids reconstructing it from git log later.
- Around line 150-156: Update the executable `editJiraIssue` example in the
Existing issues step to first read each issue’s current `fixVersions`, then
submit a deduplicated union of those entries and `[Electron] <version>`.
Preserve all existing fix-version associations while retaining the current issue
update flow.
- Around line 152-154: Add the same appropriate language identifier, such as
text, to both fenced code blocks in .claude/skills/ship-release/SKILL.md: the
anchor block at lines 152-154 and the sibling block at lines 160-164; no other
content changes are needed.
---
Nitpick comments:
In `@scripts/releaseTag.lib.ts`:
- Line 13: Remove the redundant “Strip leading 'v'...” comment near
normalizeTag, leaving the self-documenting normalizeTag implementation and its
conditional unchanged.
🪄 Autofix
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: ef5da282-6395-496c-aa88-ab38e84eae12
📒 Files selected for processing (6)
.claude/skills/ship-release/SKILL.mdAGENTS.mdjest.config.jsscripts/release-tag.tsscripts/releaseTag.lib.spec.tsscripts/releaseTag.lib.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: check (windows-latest)
- GitHub Check: check (ubuntu-latest)
🧰 Additional context used
📓 Path-based instructions (10)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Use camelCase for file names and PascalCase for component names.
Prefer editing existing files over creating abstractions unless the abstraction removes real complexity or matches an existing pattern.
Use measurable descriptions in documentation and PRs; do not use subjective descriptors or invent metrics.
Files:
jest.config.jsscripts/releaseTag.lib.spec.tsscripts/releaseTag.lib.tsAGENTS.mdscripts/release-tag.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid unnecessary comments; prefer self-documenting code through clear naming.
Files:
jest.config.jsscripts/releaseTag.lib.spec.tsscripts/releaseTag.lib.tsscripts/release-tag.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript for all new code unless explicitly told otherwise.
Use Fuselage components for all UI work; create custom components only when Fuselage lacks the required functionality.
Import Fuselage components from@rocket.chat/fuselage.
Use only valid color tokens documented byTheme.d.ts.
Use optional chaining with fallbacks for platform-specific APIs, especially Linux-only process APIs such asprocess.getuid(),getgid(),geteuid(), andgetegid().
Use TypeScript strict mode.
Redux actions must follow the Flux Standard Action pattern.
Use camelCase for file names and PascalCase for component names.
Avoid unnecessary comments; prefer self-documenting code through clear naming.
Do not commit or push without explicit user permission.
Verify library APIs, props, tokens, and types against official documentation and.d.tsfiles instead of assuming they are valid.
**/*.{ts,tsx}: Use TypeScript strict mode for all new TypeScript code unless explicitly instructed otherwise.
Prefer optional chaining and fallbacks for platform-specific APIs; only mock Linux-only APIs such asprocess.getuid(),getgid(),geteuid(), andgetegid()when defensive coding is not possible.
Use React functional components with hooks.
Redux actions must follow the Flux Standard Action shape.
When modifying a symbol, run GitNexus impact analysis first; warn about HIGH or CRITICAL risk, and rundetect_changes()before committing.
Files:
scripts/releaseTag.lib.spec.tsscripts/releaseTag.lib.tsscripts/release-tag.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfor renderer process tests.
Files:
scripts/releaseTag.lib.spec.ts
scripts/releaseTag.lib.{ts,spec.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Keep pure tag/channel logic in
releaseTag.lib.tsand unit test it inreleaseTag.lib.spec.ts.
Files:
scripts/releaseTag.lib.spec.tsscripts/releaseTag.lib.ts
**/*.{tsx,jsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Verify Fuselage props, APIs, color tokens, and other library interfaces against official documentation,
Theme.d.ts, or local declaration files instead of assuming them.
Files:
scripts/releaseTag.lib.spec.tsscripts/releaseTag.lib.tsscripts/release-tag.ts
**/*.{spec.ts,spec.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
*.spec.tsor*.spec.tsxfor renderer specifications, and place them in Jest-matched nested paths such assrc/<module>/<subdir>/orsrc/<module>/renderer.spec.tsx; do not use flatsrc/<module>/*.spec.tsfiles.
Files:
scripts/releaseTag.lib.spec.ts
scripts/releaseTag.lib.spec.ts
📄 CodeRabbit inference engine (AGENTS.md)
Run script specifications under their dedicated Jest project with
testEnvironment: 'node'.
Files:
scripts/releaseTag.lib.spec.ts
**/*.{md,mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{md,mdx}: Avoid subjective descriptors and use measurable descriptions.
Never invent metrics; use only numbers from actual logs, error messages, or documented sources.
PR descriptions should use straightforward language and focus on what changed and why.
Files:
AGENTS.md
scripts/release-tag.ts
📄 CodeRabbit inference engine (AGENTS.md)
Create release tags only with
yarn release:tag; never hand-rollgit tagandgit push. Treat guard failures as findings and do not override safety guards without explicit release-owner approval.
Files:
scripts/release-tag.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T14:15:55.541Z
Learning: Run repository commands from the repository root; do not run `yarn build` inside workspace directories.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T14:15:55.541Z
Learning: Never commit or push without explicit user permission; never commit directly to `master` or `dev`; show what will be committed before committing.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T14:15:55.541Z
Learning: Understand existing code before changing it, verify work with appropriate tests and type checks, and diagnose failures before trying repeated alternatives.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T14:15:55.541Z
Learning: Do not rename symbols with find-and-replace; use GitNexus rename, and do not edit functions, classes, or methods before impact analysis.
🪛 ast-grep (0.45.1)
scripts/release-tag.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 { execSync, execFileSync } from '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 { execSync, execFileSync } from '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 { execSync, execFileSync } from '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 { execSync, execFileSync } from '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 { execSync, execFileSync } from '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 { execSync, execFileSync } from 'child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🪛 LanguageTool
.claude/skills/ship-release/SKILL.md
[style] ~179-~179: Consider a more concise word here.
Context: ... global or project administrator rights in order to modify versions"_). Never report this a...
(IN_ORDER_TO_PREMIUM)
🪛 markdownlint-cli2 (0.23.2)
.claude/skills/ship-release/SKILL.md
[warning] 152-152: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 160-160: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (5)
scripts/releaseTag.lib.ts (1)
1-10: LGTM!Also applies to: 17-76
scripts/releaseTag.lib.spec.ts (1)
1-133: LGTM!jest.config.js (1)
76-81: LGTM!scripts/release-tag.ts (1)
3-46: LGTM!Also applies to: 71-73, 87-100, 118-229
AGENTS.md (1)
52-89: LGTM!
| ``` | ||
| Cross-check every Phase 1 PR against the issue set and report leftovers. Auditing by scraping `/pull/` links out of descriptions gives false positives — older tickets describe fixes in prose without links, so map those by hand before claiming a gap. | ||
|
|
||
| Keeping this current per-release is far cheaper than reconstructing it from `git log` months later. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the subjective cost claim.
“Far cheaper” gives an unsupported comparison. State the operational result instead, such as avoiding later reconstruction from git log.
As per coding guidelines: “Avoid subjective descriptors and use measurable descriptions. Never invent metrics.”
🤖 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 @.claude/skills/ship-release/SKILL.md at line 190, Update the release
documentation sentence to remove the subjective “far cheaper” comparison and
state only the operational outcome: keeping the information current per release
avoids reconstructing it from git log later.
Source: Coding guidelines
The skill said the fixVersions write replaces the array, but the executable example right above it sent only the new version — following it drops any release an issue already shipped in. Show the read-then- union flow instead, and label the fenced blocks (MD040). Also drop two comments that restate what normalizeTag already says.
The Windows signing flow verified gcloud auth by running `gcloud auth print-access-token` through `exec.exec`, which streams child stdout straight into the Actions log. GitHub cannot mask the value because the token is minted at runtime and is not a registered secret, so a live 1024-character access token for the code-signing service account was published in cleartext on every tagged release build. Buffer the command through the existing `runAndBuffer` helper (spawn without stdio: 'inherit'), register the value with core.setSecret as defense in depth, throw when the token comes back empty so auth failures still fail loudly, and log only its length.
Three sources accounted for most of the error-shaped output in green runs: - The chocolatey installs for openjdk11 and jsign printed ~150 download progress lines each. Pass --no-progress. - Our own patch to @kayahr/jest-electron-runner replaced upstream's silent catch with console.error, so every teardown logged "Failed to kill process: kill ESRCH/EPERM" — ~200 lines per run. The code kills the process group by negative PID and then always calls a redundant child.kill() on an already-dead PID, so the failure is expected and benign. Restore the silent catch and keep the win32 taskkill branch the patch added. - Two specs exercise error paths and let the expected warnings print. Silence them with the jest.spyOn(console, ...) convention already used elsewhere in supportedVersions/main.main.spec.ts, asserting the call where it adds value. Verified: 127/127 tests pass in the two touched specs, console blocks 150 -> 0, patch re-applies cleanly.
The macOS test job died with "Ineffective mark-compacts near heap limit" at 2038/2051 MB after 140 suites had passed and none had failed. Node was sitting on its default ~2 GB old-space ceiling: every suite runs in one process (--maxWorkers=1, load-bearing for the Electron runner) and --detectOpenHandles retains async resource references by design in order to report them. Raise the ceiling to 6 GB on the Test steps and drop --detectOpenHandles from the default scripts, keeping it available as `yarn test:debug`. Separately, the release action declared `using: 'node12'`, a runtime removed from Actions in 2023; CI coerced it and reported it as targeting Node 20, which is scheduled for removal from runners in fall 2026. Move it to node24 and bump every SHA-pinned GitHub-owned action to a node24 release. We do not use setup-gcloud's removed skip_tool_cache input, do not rely on setup-node's dummy NODE_AUTH_TOKEN, already pass CODECOV_TOKEN explicitly, and our github-script usage is limited to fs/path plus core.setOutput, so no input migration is required. Also stop addCertToStore from logging a scary "Certificate file not found" line when no path was provided at all; a genuinely missing file still reports. Note: raising the ceiling does not stop the runner accumulating retained disposables for the whole run, so this will recur as the suite grows.
Regenerating the @kayahr/jest-electron-runner patch to drop its console.error broke `yarn install` on all three platforms: patch-package could no longer apply the file and the workspace build exited 1, so CI failed before running any test. My local check was invalid. I verified the regenerated patch against a plain node_modules directory, but CI applies it during yarn's build step against a zip-backed virtual path, so a clean local apply proved nothing about CI. Restore the exact patch bytes that were passing. This brings back the ~200 "Failed to kill process: kill ESRCH/EPERM" lines per run; a noisy install that works beats a quiet one that does not. Silencing them needs a different approach that does not touch the patch context.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/pull-request-build.yml:
- Around line 40-42: Disable checkout credential persistence in both workflows:
add persist-credentials: false beside fetch-depth in
.github/workflows/pull-request-build.yml (lines 40-42), and add a with block
containing persist-credentials: false for actions/checkout in
.github/workflows/powershell-lint.yml (lines 29-30).
In `@workspaces/desktop-release-action/src/windows/google-cloud.ts`:
- Around line 71-73: Update runAndBuffer so its promise resolves on the child
process close event rather than exit, ensuring authenticateGcloud receives
complete stdout before trimming the token. Add a focused test covering token
capture through authenticateGcloud or runAndBuffer, verifying the full access
token is returned after stdout closes.
🪄 Autofix
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: b6a3e69c-65de-4f7c-9b21-e9717b7738ca
⛔ Files ignored due to path filters (1)
workspaces/desktop-release-action/dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (12)
.github/workflows/build-release.yml.github/workflows/powershell-lint.yml.github/workflows/pull-request-build.yml.github/workflows/validate-pr.ymlpackage.jsonpatches/@kayahr+jest-electron-runner+29.14.0.patchsrc/servers/supportedVersions/main.main.spec.tssrc/videoCallWindow/main/ipc.main.spec.tsworkspaces/desktop-release-action/action.ymlworkspaces/desktop-release-action/src/windows/certificates.tsworkspaces/desktop-release-action/src/windows/google-cloud.tsworkspaces/desktop-release-action/src/windows/signing-tools.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: check (macos-latest)
- GitHub Check: check (windows-latest)
🧰 Additional context used
📓 Path-based instructions (9)
workspaces/desktop-release-action/**
📄 CodeRabbit inference engine (CLAUDE.md)
After building
desktop-release-action, removeworkspaces/desktop-release-action/dist/dist; onlydist/index.jsis required.
Files:
workspaces/desktop-release-action/action.ymlworkspaces/desktop-release-action/src/windows/signing-tools.tsworkspaces/desktop-release-action/src/windows/google-cloud.tsworkspaces/desktop-release-action/src/windows/certificates.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript for all new code unless explicitly told otherwise.
Use Fuselage components for all UI work; create custom components only when Fuselage lacks the required functionality.
Import Fuselage components from@rocket.chat/fuselage.
Use only valid color tokens documented byTheme.d.ts.
Use optional chaining with fallbacks for platform-specific APIs, especially Linux-only process APIs such asprocess.getuid(),getgid(),geteuid(), andgetegid().
Use TypeScript strict mode.
Redux actions must follow the Flux Standard Action pattern.
Use camelCase for file names and PascalCase for component names.
Avoid unnecessary comments; prefer self-documenting code through clear naming.
Do not commit or push without explicit user permission.
Verify library APIs, props, tokens, and types against official documentation and.d.tsfiles instead of assuming they are valid.
**/*.{ts,tsx}: Use TypeScript strict mode for all new TypeScript code unless explicitly exempted.
Use React functional components with hooks.
Redux actions must follow the FSA (Flux Standard Action) shape.
Use camelCase for file names and PascalCase for component names.
Avoid unnecessary comments; prefer self-documenting code through clear naming.
Files:
src/videoCallWindow/main/ipc.main.spec.tsworkspaces/desktop-release-action/src/windows/signing-tools.tsworkspaces/desktop-release-action/src/windows/google-cloud.tsworkspaces/desktop-release-action/src/windows/certificates.tssrc/servers/supportedVersions/main.main.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfor renderer process tests.
Files:
src/videoCallWindow/main/ipc.main.spec.tssrc/servers/supportedVersions/main.main.spec.ts
**/*.main.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.main.spec.tsfor main process tests.
Files:
src/videoCallWindow/main/ipc.main.spec.tssrc/servers/supportedVersions/main.main.spec.ts
src/**/*.{spec.ts,spec.tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Renderer test files should be placed in nested module paths such as
src/<module>/<subdir>/*.spec.ts(x)so Jest discovers them.
Files:
src/videoCallWindow/main/ipc.main.spec.tssrc/servers/supportedVersions/main.main.spec.ts
**/*.{spec.ts,spec.tsx,main.spec.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Renderer specs must use
*.spec.tsor*.spec.tsx; main-process specs must use*.main.spec.ts.
Files:
src/videoCallWindow/main/ipc.main.spec.tssrc/servers/supportedVersions/main.main.spec.ts
**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,js}: Use optional chaining and fallbacks for platform-specific APIs; avoid mocking Linux-only APIs such asprocess.getuid(),getgid(),geteuid(), andgetegid()when defensive coding is possible.
Do not assume library props, APIs, colors, or tokens; verify them against official documentation or local.d.tsfiles.
Files:
src/videoCallWindow/main/ipc.main.spec.tsworkspaces/desktop-release-action/src/windows/signing-tools.tsworkspaces/desktop-release-action/src/windows/google-cloud.tsworkspaces/desktop-release-action/src/windows/certificates.tssrc/servers/supportedVersions/main.main.spec.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
Run meaningful verification for changes, including tests and type checks such as
npx tsc --noEmit; do not mark work complete without proving correctness.
Files:
src/videoCallWindow/main/ipc.main.spec.tsworkspaces/desktop-release-action/src/windows/signing-tools.tsworkspaces/desktop-release-action/src/windows/google-cloud.tsworkspaces/desktop-release-action/src/windows/certificates.tssrc/servers/supportedVersions/main.main.spec.ts
patches/**
📄 CodeRabbit inference engine (CLAUDE.md)
Use
patch-packageinpatches/for@kayahr/jest-electron-runner; do not use this directory for@ewsjs/xhr.
Files:
patches/@kayahr+jest-electron-runner+29.14.0.patch
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T16:57:41.258Z
Learning: Run root commands from the repository root; never run `yarn build` inside workspace directories.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T16:57:41.258Z
Learning: Do not add `ewsjs/xhr` patches to `patches/`; use `.yarn/patches/` for the Yarn patch protocol and `patches/` for `patch-package` patches.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T16:57:41.258Z
Learning: Windows builds must include `x64`, `ia32`, and `arm64` architectures.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T16:57:41.258Z
Learning: Build packages without signing first, then sign them with `jsign` using Google Cloud KMS in a separate phase.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T16:57:41.258Z
Learning: Always create release tags with `yarn release:tag`; never hand-roll `git tag` and `git push`.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T16:57:41.258Z
Learning: Tag the squashed merge commit on `master`, use bare version tag names without a `v` prefix, and ensure the tag matches `package.json`'s version.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T16:57:41.258Z
Learning: Treat release guard failures as findings; do not use `--force` or `--allow-detached-from-master` without explicit release-owner agreement.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T16:57:41.258Z
Learning: Never commit or push without explicit user permission; never commit directly to `master` or `dev`; show what will be committed first.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T16:57:41.258Z
Learning: Use worktrees to avoid disrupting another working directory.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T16:57:41.258Z
Learning: Understand existing code before changing it, diagnose failed approaches before retrying, and prefer editing existing files unless a new abstraction removes real complexity or matches an existing pattern.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T16:57:41.258Z
Learning: Run GitNexus impact analysis before editing symbols, warn on HIGH or CRITICAL risk, run `detect_changes()` before committing, and never rename symbols with find-and-replace.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T16:57:41.258Z
Learning: Avoid subjective descriptors and invented metrics; use measurable descriptions supported by actual logs or documentation.
🪛 ast-grep (0.45.1)
workspaces/desktop-release-action/src/windows/google-cloud.ts
[warning] 18-18: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(credentialsPath, gcpSaJson, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 77-79: Avoid logging sensitive data
Context: core.info(
Google Cloud authentication successful (token acquired, ${token.length} chars)
)
Note: [CWE-532] Insertion of Sensitive Information into Log File.
(log-sensitive-data-typescript)
[warning] 58-58: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(credentialsPath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 51-55: Avoid command injection
Context: exec.exec(${gcloudPath}\\gcloud.cmd, [
'auth',
'activate-service-account',
--key-file=${credentialsPath},
])
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-typescript)
[warning] 61-66: Avoid command injection
Context: exec.exec(${gcloudPath}\\gcloud.cmd, [
'config',
'set',
'project',
projectId,
])
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-typescript)
workspaces/desktop-release-action/src/windows/certificates.ts
[warning] 51-51: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(certPath, secretValue, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🪛 OpenGrep (1.26.0)
workspaces/desktop-release-action/src/windows/google-cloud.ts
[ERROR] 52-56: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 62-67: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🪛 zizmor (1.29.0)
.github/workflows/powershell-lint.yml
[warning] 29-30: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
.github/workflows/build-release.yml
[error] 37-37: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
[error] 42-42: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
🔇 Additional comments (13)
package.json (1)
43-45: LGTM!.github/workflows/build-release.yml (2)
32-39: LGTM!Also applies to: 63-63, 75-81
42-42: 🔒 Security & PrivacyNo cross-workflow cache-poisoning issue exists.
pull_requestcaches userefs/pull/.../mergescope. Release runs onmaster,dev, and tags cannot restore them.> Likely an incorrect or invalid review comment..github/workflows/validate-pr.yml (1)
30-34: LGTM!Also applies to: 45-58
workspaces/desktop-release-action/src/windows/google-cloud.ts (1)
11-18: LGTM!Also applies to: 41-44
workspaces/desktop-release-action/src/windows/certificates.ts (1)
6-23: LGTM!Also applies to: 25-55, 57-83, 85-122
workspaces/desktop-release-action/src/windows/signing-tools.ts (1)
10-32: LGTM!Also applies to: 34-68
workspaces/desktop-release-action/action.yml (1)
61-62: 🗄️ Data Integrity & IntegrationNo bundle update is required.
workspaces/desktop-release-action/dist/index.jscontains the token validation, secret masking, and installer changes.patches/@kayahr+jest-electron-runner+29.14.0.patch (1)
2-5: LGTM!Also applies to: 14-24
src/servers/supportedVersions/main.main.spec.ts (1)
100-117: LGTM!src/videoCallWindow/main/ipc.main.spec.ts (1)
340-345: LGTM!Also applies to: 355-355, 373-376
.github/workflows/pull-request-build.yml (2)
44-50: LGTM!Also applies to: 84-84, 620-620
262-268: 🎯 Functional CorrectnessKeep the current step order.
setup-gcloudonly installs the CLI. The workflow creates the credentials file before this step and authenticatesgcloudbefore using it. No ordering issue exists.> Likely an incorrect or invalid review comment.
| uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
| with: | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
for f in .github/workflows/pull-request-build.yml .github/workflows/powershell-lint.yml; do
echo "===== $f ====="
wc -l "$f"
cat -n "$f"
done
echo "===== Git-related references in workflows ====="
rg -n -i 'git |git$|github_token|token|persist-credentials|credentials|pull-requests:|permissions:|uses: actions/checkout' .github/workflows
echo "===== actions/checkout tag and pin resolution ====="
if command -v gh >/dev/null 2>&1; then
ref_json="$(gh api repos/actions/checkout/git/refs/tags/v7.0.1)"
printf '%s\n' "$ref_json" | jq .
type="$(printf '%s\n' "$ref_json" | jq -r '.object.type')"
sha="$(printf '%s\n' "$ref_json" | jq -r '.object.sha')"
if [ "$type" = tag ]; then
sha="$(gh api "repos/actions/checkout/git/tags/$sha" --jq '.object.sha')"
fi
echo "dereferenced commit: $sha"
gh api "repos/actions/checkout/commits/$sha" --jq '.sha'
gh api repos/actions/checkout/contents/action.yml?ref=v7.0.1 --jq '.content' | base64 -d | sed -n '1,180p'
else
echo "gh is unavailable"
fiRepository: RocketChat/Rocket.Chat.Electron
Length of output: 45050
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
files = [
Path(".github/workflows/pull-request-build.yml"),
Path(".github/workflows/powershell-lint.yml"),
]
for path in files:
lines = path.read_text().splitlines()
checkout = next(i for i, line in enumerate(lines) if "uses: actions/checkout@" in line)
end = next(
(i for i in range(checkout + 1, len(lines))
if re.match(r"^\s{6}- name:|^\s{6}- uses:", lines[i])),
len(lines),
)
block = lines[checkout:end]
print(f"FILE={path}")
print(f"CHECKOUT_LINE={checkout + 1}")
print("CHECKOUT_HAS_PERSIST_FALSE=" + str(any("persist-credentials: false" in line for line in block)))
print("CHECKOUT_WITH_LINES=" + " | ".join(line.strip() for line in block[1:8] if line.strip()))
permission_lines = [
line.strip() for line in lines
if re.match(r"^\s{4}(contents|pull-requests):", line)
]
print("JOB_PERMISSIONS=" + " | ".join(permission_lines))
later_git = [
(i + 1, line.strip()) for i, line in enumerate(lines[checkout + 1:], checkout + 1)
if re.search(r"(^|\s)git(\s|$)", line, re.IGNORECASE)
]
print("LATER_GIT_REFERENCES=" + " | ".join(f"{n}:{text}" for n, text in later_git) or "<none>")
later_steps = [
line.strip() for line in block
if re.match(r"^\s*(run:|uses:)", line)
]
print("STEPS_AFTER_CHECKOUT=" + " | ".join(later_steps))
PYRepository: RocketChat/Rocket.Chat.Electron
Length of output: 688
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
files = [
Path(".github/workflows/pull-request-build.yml"),
Path(".github/workflows/powershell-lint.yml"),
]
step_start = re.compile(r"^(?P<indent>\s*)-\s+(?:name|uses):")
for path in files:
lines = path.read_text().splitlines()
starts = [(i, len(m.group("indent"))) for i, line in enumerate(lines)
if (m := step_start.match(line))]
checkout_i, checkout_indent = next(
(i, indent) for i, indent in starts
if "uses: actions/checkout@" in lines[i]
)
checkout_end = next(
(i for i, indent in starts if i > checkout_i and indent == checkout_indent),
len(lines),
)
later_end = [
line.strip() for line in lines[checkout_end:]
if re.match(r"^\s+(?:run|uses):", line)
]
later_git = [
(i + 1, line.strip()) for i, line in enumerate(lines[checkout_i + 1:], checkout_i + 1)
if re.search(r"(^|\s)git(\s|$)", line, re.IGNORECASE)
]
permissions = [
line.strip() for line in lines
if re.match(r"^\s{6}(?:contents|pull-requests):", line)
]
checkout_block = lines[checkout_i:checkout_end]
print(f"FILE={path}")
print(f"CHECKOUT_LINE={checkout_i + 1}")
print("CHECKOUT_BLOCK=" + " | ".join(line.strip() for line in checkout_block))
print("JOB_PERMISSIONS=" + " | ".join(permissions) or "<none>")
print("LATER_RUN_OR_USES=" + " | ".join(later_end) or "<none>")
print("LATER_GIT_REFERENCES=" + " | ".join(
f"{line_no}:{text}" for line_no, text in later_git
) or "<none>")
PYRepository: RocketChat/Rocket.Chat.Electron
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
for path in [
Path(".github/workflows/pull-request-build.yml"),
Path(".github/workflows/powershell-lint.yml"),
]:
lines = path.read_text().splitlines()
checkout_i = next(i for i, line in enumerate(lines)
if "uses: actions/checkout@" in line)
checkout_indent = len(lines[checkout_i]) - len(lines[checkout_i].lstrip())
step_i = max(
i for i in range(checkout_i + 1)
if re.match(r"^\s*-\s+name:", lines[i])
and len(lines[i]) - len(lines[i].lstrip()) == checkout_indent
)
next_step = next(
(i for i in range(checkout_i + 1, len(lines))
if re.match(r"^\s*-\s+name:", lines[i])
and len(lines[i]) - len(lines[i].lstrip()) == checkout_indent),
len(lines),
)
permissions = [
line.strip() for line in lines
if re.match(r"^\s{6}(?:contents|pull-requests):", line)
]
later_steps = [
line.strip() for line in lines[next_step:]
if re.match(r"^\s+(?:run|uses):", line)
]
later_git = [
(i + 1, line.strip()) for i, line in enumerate(lines[checkout_i + 1:], checkout_i + 1)
if re.search(r"(^|\s)git(\s|$)", line, re.IGNORECASE)
]
print(f"FILE={path}")
print(f"CHECKOUT_STEP_LINE={step_i + 1}")
print("CHECKOUT_BLOCK=" + " | ".join(
line.strip() for line in lines[step_i:next_step]
))
print("JOB_PERMISSIONS=" + " | ".join(permissions) or "<none>")
print("LATER_RUN_OR_USES=" + " | ".join(later_steps) or "<none>")
print("LATER_GIT_REFERENCES=" + " | ".join(
f"{line_no}:{text}" for line_no, text in later_git
) or "<none>")
PYRepository: RocketChat/Rocket.Chat.Electron
Length of output: 286
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
step_re = re.compile(r"^(?P<indent>\s*)-\s+name:")
for path in (
Path(".github/workflows/pull-request-build.yml"),
Path(".github/workflows/powershell-lint.yml"),
):
lines = path.read_text().splitlines()
checkout_i = next(i for i, line in enumerate(lines)
if "uses: actions/checkout@" in line)
step_i = max(
i for i in range(checkout_i + 1)
if step_re.match(lines[i])
)
step_indent = len(step_re.match(lines[step_i]).group("indent"))
next_step = next(
(i for i in range(checkout_i + 1, len(lines))
if (m := step_re.match(lines[i])) and len(m.group("indent")) == step_indent),
len(lines),
)
permissions = [
line.strip() for line in lines
if re.match(r"^\s{6}(?:contents|pull-requests):", line)
]
later_steps = [
line.strip() for line in lines[next_step:]
if re.match(r"^\s+(?:run|uses):", line)
]
later_git = [
(i + 1, line.strip()) for i, line in enumerate(lines[checkout_i + 1:], checkout_i + 1)
if re.search(r"(^|\s)git(\s|$)", line, re.IGNORECASE)
]
print(f"FILE={path}")
print(f"CHECKOUT_STEP_LINE={step_i + 1}")
print("CHECKOUT_BLOCK=" + " | ".join(
line.strip() for line in lines[step_i:next_step]
))
print("JOB_PERMISSIONS=" + " | ".join(permissions) or "<none>")
print("LATER_RUN_OR_USES=" + " | ".join(later_steps) or "<none>")
print("LATER_GIT_REFERENCES=" + " | ".join(
f"{line_no}:{text}" for line_no, text in later_git
) or "<none>")
PYRepository: RocketChat/Rocket.Chat.Electron
Length of output: 1680
Disable checkout credential persistence in both workflows.
actions/checkout defaults persist-credentials to true and stores github.token in the local Git configuration. No later authenticated Git operation requires this token. The pull-request workflow runs PR-controlled commands with pull-requests: write, which increases exposure impact.
- Add
persist-credentials: falsebesidefetch-depth: 0in.github/workflows/pull-request-build.yml. - Add
with: persist-credentials: falsein.github/workflows/powershell-lint.yml.
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 39-42: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
📍 Affects 2 files
.github/workflows/pull-request-build.yml#L40-L42(this comment).github/workflows/powershell-lint.yml#L29-L30
🤖 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 @.github/workflows/pull-request-build.yml around lines 40 - 42, Disable
checkout credential persistence in both workflows: add persist-credentials:
false beside fetch-depth in .github/workflows/pull-request-build.yml (lines
40-42), and add a with block containing persist-credentials: false for
actions/checkout in .github/workflows/powershell-lint.yml (lines 29-30).
Source: Linters/SAST tools
| const token = ( | ||
| await runAndBuffer(`"${gcloudPath}\\gcloud.cmd" auth print-access-token`) | ||
| ).trim(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 "p\.once\('(exit|close)'" \
workspaces/desktop-release-action/src/shell.ts
rg -n -C 8 "authenticateGcloud|print-access-token|runAndBuffer" \
workspaces/desktop-release-action/src/windows/google-cloud.tsRepository: RocketChat/Rocket.Chat.Electron
Length of output: 3189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n workspaces/desktop-release-action/src/shell.ts | sed -n '1,90p'
printf '\n--- related tests and scripts ---\n'
rg -n --glob '!*dist*' "runAndBuffer|shell\.ts|desktop-release-action" \
workspaces/desktop-release-action package.json .github 2>/dev/null | head -200
printf '\n--- standalone event-order probe ---\n'
node - <<'JS'
const { spawn } = require('node:child_process');
const child = spawn(
process.execPath,
['-e', `
process.stdout.write('token-part-1');
setTimeout(() => process.stdout.write('token-part-2'), 25);
`],
{ stdio: ['ignore', 'pipe', 'ignore'] },
);
let output = '';
child.stdout.on('data', (chunk) => {
output += chunk;
console.log('data:', JSON.stringify(chunk.toString()));
});
child.once('exit', (code) => {
console.log('exit:', code, 'captured:', JSON.stringify(output));
});
child.once('close', (code) => {
console.log('close:', code, 'captured:', JSON.stringify(output));
});
JSRepository: RocketChat/Rocket.Chat.Electron
Length of output: 4231
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const { spawn } = require('node:child_process');
const command = `${process.execPath} -e "setTimeout(() => process.stdout.write('late-token'), 50)" & exit 0`;
const child = spawn(command, { shell: true, stdio: ['ignore', 'pipe', 'ignore'] });
let output = '';
const events = [];
child.stdout.on('data', (chunk) => {
output += chunk;
events.push(`data:${chunk.toString()}`);
});
child.once('exit', (code) => {
events.push(`exit:${code}:capture=${JSON.stringify(output)}`);
});
child.once('close', (code) => {
events.push(`close:${code}:capture=${JSON.stringify(output)}`);
console.log(events.join('\n'));
});
JSRepository: RocketChat/Rocket.Chat.Electron
Length of output: 232
Resolve runAndBuffer after stdout closes.
runAndBuffer resolves on exit, so authenticateGcloud can read an incomplete access token. Resolve on close and add a focused token-capture test.
🤖 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 `@workspaces/desktop-release-action/src/windows/google-cloud.ts` around lines
71 - 73, Update runAndBuffer so its promise resolves on the child process close
event rather than exit, ensuring authenticateGcloud receives complete stdout
before trimming the token. Add a focused test covering token capture through
authenticateGcloud or runAndBuffer, verifying the full access token is returned
after stdout closes.
Two defects surfaced while cleaning CI log noise. Both were logging real
warnings on every TabBar render, so they were fixed rather than silenced.
WorkspaceTab mounted the Favicon img unconditionally and hid it with
`display: none` via a `visible` prop, passing `src={favicon ?? ''}` when there
was no favicon. React warns "An empty string was passed to the src attribute"
for every such render, and the browser still has an img element in the DOM.
Render it only when a favicon exists; Initials already covers the fallback.
useTabBarLayout built its `tabListRef` callback inline, so it got a new
identity on every render. React detaches and reattaches a callback ref whose
identity changed, so each `setAvailableWidth` re-render ran unobserve/observe
again and scheduled a redundant requestAnimationFrame — which in tests fired
after the render's act() scope had closed, producing "not wrapped in act"
warnings. Wrap it in useCallback so the identity is stable.
Verified: 67/67 tests pass across the four TabBar specs plus
ui/main/rootWindow.spec.ts; console blocks in TabBar/index.spec.tsx go from 65
to 0, with no act() or empty-src warnings left.
The macOS OOM fix raises Node's old-space limit; it does not stop the Electron runner accumulating one retained teardown closure per spawned child for the whole run. Document the mechanism, why macOS fails first, why workerIdleMemoryLimit does not apply at --maxWorkers=1, and that the real fix is upstream per-target disposal — so the next person hitting this does not simply raise the number again.
Second attempt; the first broke `yarn install` on all three platforms and was reverted in 3fddc52. The patched teardown kills the process group by negative PID (or taskkill on win32) and then always calls a redundant child.kill('SIGKILL') on an already-dead PID, so ESRCH/EPERM is the expected outcome. Our patch logged it via console.error, emitting ~199 matched lines per run — each a ~20-line block with a full stack trace. Upstream 29.15.0 has no such logging and uses a silent catch; only the win32 taskkill branch is genuinely ours, so the noise was entirely self-inflicted. Make the catch silent and keep the branching. Verified the way the previous attempt was not: wiped node_modules entirely and ran a full `yarn install` (exit 0, no patch failure), then confirmed against a confirmed-pristine copy of the package that the patch applies and yields taskkill=2 / console.error=0. The patch's index hash and context lines are byte-identical to the previously-passing version; only added lines and the hunk's new-length differ.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/KNOWN_ISSUES.md`:
- Around line 138-139: Update the documentation around the macOS runner
explanation and the lines covering “buys headroom” and “real fix” to remove
subjective descriptors. Replace them with directly observable results, measured
comparisons, or explicit required actions, while preserving the existing build
and test context.
- Around line 140-142: Update the runner-memory qualification in the workaround
entry to state that the documented memory applies to the standard ubuntu-latest,
macos-latest, and windows-latest GitHub-hosted runners, and add the GitHub
runner specifications link as the citation. Remove the unqualified
“GitHub-hosted runners have >= 7 GB” claim while preserving the existing
NODE_OPTIONS guidance.
🪄 Autofix
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: 538d8a37-da91-46af-85e2-7ce1d8d6d3c3
📒 Files selected for processing (4)
docs/KNOWN_ISSUES.mdpatches/@kayahr+jest-electron-runner+29.14.0.patchsrc/ui/components/TabBar/WorkspaceTab.tsxsrc/ui/components/TabBar/useTabBarLayout.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- patches/@kayahr+jest-electron-runner+29.14.0.patch
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript for all new code unless explicitly told otherwise.
Use Fuselage components for all UI work; create custom components only when Fuselage lacks the required functionality.
Import Fuselage components from@rocket.chat/fuselage.
Use only valid color tokens documented byTheme.d.ts.
Use optional chaining with fallbacks for platform-specific APIs, especially Linux-only process APIs such asprocess.getuid(),getgid(),geteuid(), andgetegid().
Use TypeScript strict mode.
Redux actions must follow the Flux Standard Action pattern.
Use camelCase for file names and PascalCase for component names.
Avoid unnecessary comments; prefer self-documenting code through clear naming.
Do not commit or push without explicit user permission.
Verify library APIs, props, tokens, and types against official documentation and.d.tsfiles instead of assuming they are valid.
**/*.{ts,tsx}: Use optional chaining and fallbacks for platform-specific APIs; specifically guard Linux-only APIs such asprocess.getuid(),getgid(),geteuid(), andgetegid().
Use React functional components with hooks.
Redux actions must follow the FSA (Flux Standard Action) shape.
Use camelCase for file names and PascalCase for component names.
Avoid unnecessary comments; prefer self-documenting code through clear naming.
Files:
src/ui/components/TabBar/WorkspaceTab.tsxsrc/ui/components/TabBar/useTabBarLayout.ts
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use React functional components with hooks.
**/*.{tsx,jsx}: Use Fuselage components from@rocket.chat/fuselagefor UI work unless the design requires functionality Fuselage does not provide.
Verify Fuselage props, APIs, and color tokens against official documentation, local.d.tsfiles, andTheme.d.tsrather than assuming they are valid.
UI changes require runtime or visual verification; component tests alone are insufficient for validating painting, clipping, SVG rendering, or layout.
Files:
src/ui/components/TabBar/WorkspaceTab.tsx
**/*.{md,mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{md,mdx}: Avoid subjective descriptors and use measurable descriptions.
Never invent metrics; use only numbers from actual logs, error messages, or documented sources.
PR descriptions should use straightforward language and focus on what changed and why.
Files:
docs/KNOWN_ISSUES.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T17:41:16.727Z
Learning: Use TypeScript strict mode for all new code unless explicitly told otherwise.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T17:41:16.727Z
Learning: Run repository commands from the root; do not run `yarn build` inside workspace directories.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T17:41:16.727Z
Learning: Build packages without signing first, then sign the built packages with `jsign` using Google Cloud KMS.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T17:41:16.727Z
Learning: Prefer editing existing files over introducing new abstractions unless the abstraction removes real complexity or matches an existing pattern.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T17:41:16.727Z
Learning: Never commit or push without explicit user permission; never commit directly to `master` or `dev`; show what will be committed before committing.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T17:41:16.727Z
Learning: Understand the existing implementation before changing it, verify work with meaningful checks, diagnose failures before iterating, and verify library APIs against official documentation or local `.d.ts` files.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T17:41:16.727Z
Learning: Before editing any function, class, or method, run GitNexus upstream impact analysis and report the blast radius; warn the user for HIGH or CRITICAL risk.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T17:41:16.727Z
Learning: Run GitNexus `detect_changes()` before committing to verify that changes affect only expected symbols and execution flows.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T17:41:16.727Z
Learning: Never rename symbols with find-and-replace; use GitNexus `rename` so the call graph is understood.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-11T17:41:16.727Z
Learning: Avoid subjective descriptors and invented metrics in documentation and PR descriptions; use measurable descriptions backed by actual logs or documented sources.
🪛 LanguageTool
docs/KNOWN_ISSUES.md
[uncategorized] ~140-~140: The official name of this software platform is spelled with a capital “H”.
Context: ...-space-size=6144on theTeststep in .github/workflows/validate-pr.ymland.github...
(GITHUB)
[uncategorized] ~141-~141: The official name of this software platform is spelled with a capital “H”.
Context: ....github/workflows/validate-pr.yml and .github/workflows/build-release.yml (GitHub-...
(GITHUB)
[style] ~143-~143: Consider a more concise word here.
Context: ...ins async resource references by design in order to report them. It is still available as...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~150-~150: The official name of this software platform is spelled with a capital “H”.
Context: ...o worker to recycle. - Affected files: .github/workflows/validate-pr.yml, .github/work...
(GITHUB)
[uncategorized] ~150-~150: The official name of this software platform is spelled with a capital “H”.
Context: ...es: .github/workflows/validate-pr.yml, .github/workflows/build-release.yml, package....
(GITHUB)
🔇 Additional comments (3)
docs/KNOWN_ISSUES.md (1)
124-137: LGTM!Also applies to: 148-153
src/ui/components/TabBar/WorkspaceTab.tsx (1)
227-234: 🎯 Functional CorrectnessRun runtime or visual verification for both favicon states.
Verify that a truthy
faviconrendersFavicon, while a falsy value rendersInitialswithout an empty image element. Check both horizontal and vertical layouts.As per coding guidelines:
**/*.{tsx,jsx}UI changes require runtime or visual verification; component tests alone are insufficient for validating painting, clipping, SVG rendering, or layout.Source: Coding guidelines
src/ui/components/TabBar/useTabBarLayout.ts (1)
1-1: LGTM!Also applies to: 73-87
| before `yarn test` (build-release) does. macOS fails first: slowest arm64 runner, largest | ||
| retained set. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace subjective wording with measurable statements.
Lines 138-139 describe the runner as “slowest” and having the “largest retained set” without measured comparisons. Lines 145-147 use “buys headroom” and “real fix”. State the observed result and required action directly.
Proposed wording
- macOS fails first: slowest arm64 runner, largest retained set.
+ In the referenced run, `macOS-latest` failed first. The observed heap values are listed above.
...
- raising the ceiling buys headroom, it does not stop the accumulation.
+ increasing the heap limit does not stop the accumulation.
- the real fix is upstream disposing each child
+ the required upstream fix is to dispose each childAs per coding guidelines: **/*.{md,mdx} must avoid subjective descriptors and use measurable descriptions.
Also applies to: 145-147
🤖 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/KNOWN_ISSUES.md` around lines 138 - 139, Update the documentation around
the macOS runner explanation and the lines covering “buys headroom” and “real
fix” to remove subjective descriptors. Replace them with directly observable
results, measured comparisons, or explicit required actions, while preserving
the existing build and test context.
Source: Coding guidelines
| - Workaround: `NODE_OPTIONS: --max-old-space-size=6144` on the `Test` step in | ||
| `.github/workflows/validate-pr.yml` and `.github/workflows/build-release.yml` | ||
| (GitHub-hosted runners have >= 7 GB), and `--detectOpenHandles` removed from the `test` / |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- relevant documentation ---'
cat -n docs/KNOWN_ISSUES.md | sed -n '125,160p'
printf '%s\n' '--- workflow files ---'
git ls-files '.github/workflows/*' | sort
printf '%s\n' '--- runner declarations and memory references ---'
rg -n -i --glob '.github/workflows/**' \
'runs-on|ubuntu-|windows-|macos-|max-old-space-size|7 GB|memory|runner' \
.github/workflows || trueRepository: RocketChat/Rocket.Chat.Electron
Length of output: 7956
🌐 Web query:
GitHub-hosted runners standard runner specifications memory ubuntu-latest macos-latest windows-latest 7 GB
💡 Result:
The memory specifications for standard GitHub-hosted runners vary depending on whether the repository is public or private, and which operating system is used. As of August 11, 2026, the specifications are as follows [1][2]: Public Repositories: - Ubuntu-latest (Linux): 16 GB RAM (4 CPUs) [1][2] - Windows-latest (Windows): 16 GB RAM (4 CPUs) [1][2] - Macos-latest (macOS M1): 7 GB RAM (3 vCPUs) [1][2] Private Repositories: - Ubuntu-latest (Linux): 8 GB RAM (2 CPUs) [1][2] - Windows-latest (Windows): 8 GB RAM (2 CPUs) [1][2] - Macos-latest (macOS M1): 7 GB RAM (3 vCPUs) [1][2] The 7 GB RAM figure specifically applies to the standard GitHub-hosted macOS (M1/arm64) runners, which provide this amount of memory for both public and private repositories [1][2]. For Linux and Windows, the memory allocation is 16 GB for public repositories and 8 GB for private ones [1][2]. Larger memory options are available through GitHub-hosted large runners [3].
Citations:
- 1: https://docs.github.com/en/actions/reference/runners/github-hosted-runners
- 2: https://docs.github.com/actions/using-jobs/choosing-the-runner-for-a-job
- 3: https://hyperenv.com/blog/github-actions-running-out-of-memory/
Cite and qualify the runner-memory claim.
The workflows use standard ubuntu-latest, macos-latest, and windows-latest runners. GitHub documents at least 7 GB of memory for these runners, with macOS arm64 providing 7 GB. Add the GitHub runner-specifications link and limit the statement to these runners.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~140-~140: The official name of this software platform is spelled with a capital “H”.
Context: ...-space-size=6144on theTeststep in .github/workflows/validate-pr.ymland.github...
(GITHUB)
[uncategorized] ~141-~141: The official name of this software platform is spelled with a capital “H”.
Context: ....github/workflows/validate-pr.yml and .github/workflows/build-release.yml (GitHub-...
(GITHUB)
🤖 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/KNOWN_ISSUES.md` around lines 140 - 142, Update the runner-memory
qualification in the workaround entry to state that the documented memory
applies to the standard ubuntu-latest, macos-latest, and windows-latest
GitHub-hosted runners, and add the GitHub runner specifications link as the
citation. Remove the unqualified “GitHub-hosted runners have >= 7 GB” claim
while preserving the existing NODE_OPTIONS guidance.
Source: Coding guidelines
The PR build's auth check captured `gcloud auth print-access-token 2>&1` into $tokenOutput and, on the failure branch, printed it. Because 2>&1 merges stdout, that variable holds the access token whenever the command produced one, so a non-zero exit after a partial success would publish live token material to a public log — the same defect already fixed in the release action. Report the exit code instead, and require a non-empty token for success so genuine auth failures are still caught.
…tch" This reverts commit 97a2b60.
`workspaces/desktop-release-action` was linted by nothing: eslint hard-errored with "couldn't determine the plugin import uniquely", so the workspace that handles code-signing credentials had no lint coverage at all. ESLint 8 walks up the directory tree and loads the root .eslintrc.json in addition to the workspace's own. Both extend @rocket.chat/eslint-config, and each resolved its own copy of eslint-plugin-import (root ~2.26.0, workspace ~2.29.1), which ESLint refuses. The root .eslintignore lists /workspaces/*, but that only filters which files are linted — it does not stop config inheritance. Set "root": true to stop the upward walk, then point parserOptions.project at the workspace's own tsconfig so the type-aware rules get parserServices (without it, prefer-optional-chain crashed on getTypeAtLocation). Config-only: no dependency versions changed and no rule disabled. The rest is that first lint pass coming back clean: import ordering, prettier wrapping, and I-prefixing three local interfaces per naming-convention. No logic changed; dist/index.js is unaffected, since the renames are type-level only.
1 participant
What
yarn release:tagcould tag the wrong tree without complaining. This makes it fail closed, and documents the tooling inAGENTS.md.Two gaps in
scripts/release-tag.ts:There was also no non-interactive path: the
Proceed? (y/N)prompt reads a real TTY, so anything scripted drifted toward hand-rolledgit tag+git push, which skips every guard the script has.Guards
package.jsonorigin/master--allow-detached-from-master--force--forceAn existing tag stays non-overridable on purpose: the remedy is bumping the version, not forcing the tag. Channel detection (stable / alpha / beta / candidate) is unchanged and still compares only within a channel, so an alpha never blocks a stable or vice versa.
Also
--yes/-yskips the prompt (auto-skipped whenCI=true), plus--help.scripts/releaseTag.lib.tsso it can be unit tested;release-tag.tskeeps the I/O. Script specs run under a new node-environment Jest project — the two existing projects are untouched.AGENTS.mdgains a Releases And Tagging section: the guard table, the flags, the tag-the-merge-commit rule, and the fresh-worktreenode_modulestrap.ship-releaseskill now callsyarn release:tag --yesinstead of carrying a manualgit tagfallback, and its Jira release sync is promoted from an optional wrap-up line to its own phase (setfixVersionon every shipped issue, create issues for work that has none, verify by JQL).Verification
npx tsc --noEmit— 0 errorsyarn lint— cleanyarn test --runTestsByPath scripts/releaseTag.lib.spec.ts— 18/18 pass--forcecorrectly failing to override an existing tag. 382 local tags unchanged; nothing tagged or pushed.Test plan
yarn release:tag --helplists the flags.origin/master,yarn release:tag --yesexits 1 and prints both SHAs before reaching the tag step.package.jsonat an already-tagged version, it exits 1 — with and without--force.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Maintenance