Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent 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). (13)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🔇 Additional comments (3)
WalkthroughAdd actionlint configuration and a VS Code-style problem matcher; add a workflow that downloads, verifies, and runs a pinned actionlint binary; remove invalid Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as "GitHub Actions Runner"
participant Repo as "Repository (.github/*)"
participant Downloader as "Downloader (curl/sha256sum)"
participant Actionlint as "actionlint binary"
Runner->>Repo: checkout repository
Runner->>Repo: register matcher (::add-matcher:: .github/actionlint-matcher.json)
Runner->>Downloader: download actionlint tarball + checksums
Downloader->>Downloader: verify checksum (sha256sum -c)
Downloader->>Runner: extract actionlint binary
Runner->>Actionlint: run ./actionlint -color
Actionlint-->>Runner: emit lint results
Runner->>Repo: matcher maps results to annotations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 #40010 +/- ##
===========================================
- Coverage 70.56% 70.54% -0.03%
===========================================
Files 3270 3270
Lines 116766 116766
Branches 21058 21089 +31
===========================================
- Hits 82397 82371 -26
- Misses 32317 32334 +17
- Partials 2052 2061 +9
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
✅ Layne — scan passed No security issues found on latest push. |
|
/jira FIPS-7 |
|
The milestone "8.4.0" does not exist on the Jira board; the task was created without Fix version. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 15 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/actionlint.yml">
<violation number="1" location=".github/workflows/actionlint.yml:12">
P1: Do not execute an unpinned remote script from `main`; pin to an immutable actionlint version/commit (and use `curl -fsSL`) to keep CI deterministic and reduce supply-chain risk.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/actionlint.yml (2)
4-6: Consider adding explicit permissions for least privilege.This workflow only needs to read repository contents. Adding an explicit
permissionsblock follows security best practices and limits potential damage if the workflow is compromised.Suggested permissions block
jobs: actionlint: runs-on: ubuntu-latest + permissions: + contents: read steps:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/actionlint.yml around lines 4 - 6, Add an explicit least-privilege permissions block to the workflow so the actionlint job only has read access to repository contents: update the workflow YAML containing the jobs -> actionlint definition (the job named "actionlint" and its surrounding top-level workflow config) to include a permissions section (e.g., permissions: contents: read) at the appropriate scope (top-level or job-level) to limit access to only what is required by the workflow.
12-12: Unpinned remote script execution poses a supply chain risk.The download script is fetched from the
mainbranch without integrity verification. If the upstream repo is compromised or the script changes unexpectedly, arbitrary code could run in your CI. This is inconsistent with the pinned checkout action on line 8.Consider one of these alternatives:
- Use the official action
rhysd/actionlint-actionwith a pinned version- Pin to a specific commit hash in the URL
- Vendor the download script or binary into your repository
Option 1: Use the official action (recommended)
- name: Check workflow files - run: | - echo "::add-matcher::.github/actionlint-matcher.json" - bash <(curl https://raw.githubusercontent.com/rhysd/actionlint/main/scripts/download-actionlint.bash) - ./actionlint -color - shell: bash + uses: rhysd/actionlint-action@v1Note: If using the action, the custom matcher may not be needed as the action handles annotations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/actionlint.yml at line 12, The workflow currently executes an unpinned remote script via the curl invocation "bash <(curl https://raw.githubusercontent.com/rhysd/actionlint/main/scripts/download-actionlint.bash)" which is a supply‑chain risk; update the workflow to either (a) replace this curl invocation with the official rhysd/actionlint-action pinned to a specific version tag (use the action name "rhysd/actionlint-action" and a fixed sha or tag), (b) change the curl URL to point to a specific commit hash (replace "main" with the commit SHA in the "download-actionlint.bash" URL) and add an integrity check, or (c) vendor the "download-actionlint.bash" script into the repo and call it locally—pick one approach and apply it wherever the curl download is used so the script is no longer fetched from an unpinned "main".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/actions/build-docker/action.yml:
- Around line 23-26: The input default for setup-docker is set to the string
'true' but the condition later compares it to a boolean true; update the
condition(s) that reference the setup-docker input (e.g., the condition at the
block currently comparing inputs.setup-docker == true) to use the string form
inputs.setup-docker == 'true' so comparisons match the string default and mirror
other inputs like publish-image.
In `@packages/release-action/action.yml`:
- Around line 12-14: The action's runs.main currently points to the TypeScript
source ("src/index.ts") under the "node24" runtime which will fail at runtime;
compile/bundle the TypeScript into distributable JavaScript (e.g., produce
"dist/index.js") and update runs.main to reference that compiled JS file (for
example set runs.main to "dist/index.js"), ensuring the build output is
committed or generated during CI so the action runs correctly.
---
Nitpick comments:
In @.github/workflows/actionlint.yml:
- Around line 4-6: Add an explicit least-privilege permissions block to the
workflow so the actionlint job only has read access to repository contents:
update the workflow YAML containing the jobs -> actionlint definition (the job
named "actionlint" and its surrounding top-level workflow config) to include a
permissions section (e.g., permissions: contents: read) at the appropriate scope
(top-level or job-level) to limit access to only what is required by the
workflow.
- Line 12: The workflow currently executes an unpinned remote script via the
curl invocation "bash <(curl
https://raw.githubusercontent.com/rhysd/actionlint/main/scripts/download-actionlint.bash)"
which is a supply‑chain risk; update the workflow to either (a) replace this
curl invocation with the official rhysd/actionlint-action pinned to a specific
version tag (use the action name "rhysd/actionlint-action" and a fixed sha or
tag), (b) change the curl URL to point to a specific commit hash (replace "main"
with the commit SHA in the "download-actionlint.bash" URL) and add an integrity
check, or (c) vendor the "download-actionlint.bash" script into the repo and
call it locally—pick one approach and apply it wherever the curl download is
used so the script is no longer fetched from an unpinned "main".
🪄 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: 92aee631-90fd-4724-9f95-b0a384e3b369
📒 Files selected for processing (15)
.github/actionlint-matcher.json.github/actions/build-docker/action.yml.github/actions/meteor-build/action.yml.github/actions/setup-node/action.yml.github/actions/setup-playwright/action.yml.github/actions/update-version-durability/action.yml.github/workflows/actionlint.yml.github/workflows/auto-close-duplicates.yml.github/workflows/ci-code-check.yml.github/workflows/ci-test-e2e.yml.github/workflows/ci-test-storybook.yml.github/workflows/ci-test-unit.yml.github/workflows/ci.yml.github/workflows/dedupe-issues.ymlpackages/release-action/action.yml
💤 Files with no reviewable changes (2)
- .github/actions/update-version-durability/action.yml
- .github/actions/setup-node/action.yml
📜 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). (7)
- GitHub Check: 🚢 Build Docker (arm64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (amd64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (amd64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (arm64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
.github/actions/setup-playwright/action.yml
📚 Learning: 2026-01-08T15:03:59.621Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38068
File: apps/meteor/tests/data/apps/app-packages/README.md:14-16
Timestamp: 2026-01-08T15:03:59.621Z
Learning: For the RocketChat/Rocket.Chat repository, do not analyze or report formatting issues (such as hard tabs vs spaces, line breaks, etc.). The project relies on automated linting tools to enforce formatting standards.
Applied to files:
.github/workflows/dedupe-issues.yml.github/workflows/ci-test-e2e.yml
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
.github/workflows/ci-test-e2e.yml
🔇 Additional comments (18)
.github/actions/setup-playwright/action.yml (1)
2-2: LGTM! Metadata improvement for actionlint compliance.The
descriptionfield is a recommended best practice for GitHub Actions composite actions and accurately describes the action's purpose. This addition aligns with the PR's objective to achieve actionlint compliance without changing runtime behavior..github/actionlint-matcher.json (1)
1-17: LGTM!The problem matcher configuration is well-structured. The regex correctly handles ANSI color escape sequences (matching the
-colorflag usage in the workflow) and properly extracts file, line, column, message, and error code from actionlint output..github/actions/meteor-build/action.yml (1)
1-2: LGTM!Adding the
descriptionfield is a good practice for composite actions as it improves discoverability and documentation..github/actions/build-docker/action.yml (1)
1-2: LGTM!The description addition provides helpful context for the composite action.
.github/workflows/ci.yml (4)
48-48: LGTM! Shell variable quoting improvements.The double-quoting of
$GITHUB_EVENT_PATHand$GITHUB_OUTPUTreferences prevents word-splitting and glob expansion issues (SC2086). These are safe and correct changes.Also applies to: 75-77, 84-92, 102-102, 111-111, 123-123
222-222: LGTM! Safer tar invocation using stdin file list.Piping
git ls-filesoutput totar -T -is more robust than command substitution, especially for repositories with many generated files. This avoids potential "argument list too long" errors.
407-429: LGTM! Correct array expansion and shellcheck directive.
- The
SC2016disable is appropriate since$digestis a jq variable, not a shell variable that needs expansion.- Using
${refs[*]}for debug echo (concatenated output) and"${refs[@]}"fordocker buildx imagetools(separate arguments) follows bash best practices.
835-856: LGTM! Deployment and publish quoting fixes.Proper quoting of variables in deployment scripts (
$ROCKET_DEPLOY_DIR,$GPG_PASSWORD) and release conditionals prevents issues with special characters and adheres to shellcheck best practices.Also applies to: 919-920, 1019-1019
.github/workflows/ci-code-check.yml (1)
67-67: LGTM! Proper quoting in test expression.Quoting
$LAUNCHERin the file existence test prevents word-splitting issues if the path contains spaces or special characters..github/workflows/ci-test-unit.yml (1)
16-19: LGTM! Optional NPM_TOKEN secret added consistently.The optional
NPM_TOKENsecret declaration allows the workflow to function for fork PRs (where secrets aren't available) while enabling private package access when the token is present.Also applies to: 42-42
.github/workflows/ci-test-storybook.yml (1)
13-17: LGTM! Optional NPM_TOKEN secret added for consistency.Aligns with the same pattern applied to other CI test workflows, enabling private package access when available.
Also applies to: 38-38
.github/workflows/ci-test-e2e.yml (4)
62-63: LGTM! Optional NPM_TOKEN secret added.Consistent with the other CI test workflows, enabling private package access for non-fork PRs.
Also applies to: 126-126
162-162: LGTM! Shell variable quoting improvements.Proper quoting of
$GITHUB_ENV,$COVERAGE_DIR, and coverage file paths prevents word-splitting issues.Also applies to: 173-174, 272-273
198-202: LGTM! Cleaner log grep pattern.Direct piping of
docker compose logstogrepis cleaner and functionally equivalent.
216-221: LGTM! Proper exit code handling ensures cleanup runs.The pattern
npm run testapi || s=$?correctly captures the exit code only on failure, allowsdocker compose stopto run unconditionally, andexit "${s:-0}"properly propagates the original exit status (0 for success, captured code for failure). This fixes the bug where failing tests could skip Docker cleanup.Also applies to: 232-237
.github/workflows/auto-close-duplicates.yml (1)
2-2: Good cleanup of workflow metadata.Removing the workflow-level
descriptionkeeps the file aligned with valid workflow schema without changing behavior..github/workflows/dedupe-issues.yml (2)
2-2: Metadata cleanup looks correct.Removing workflow-level
descriptionis aligned with valid workflow keys and does not affect runtime logic.
30-30: Themodelinput is officially supported onanthropics/claude-code-base-action@beta.Verification confirms that
modelis a supported input in the action'saction.ymland takes precedence overanthropic_modelvia the environment variable mapping. The code at this line is correct.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/actionlint.yml">
<violation number="1" location=".github/workflows/actionlint.yml:13">
P2: Add `-L` to the curl commands so redirects to the release assets are followed; otherwise the downloaded files are HTML redirect bodies and the checksum/tar steps will fail.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
cardoso
added
the
stat: QA assured
coderabbitai
bot
added
type: chore
and removed
stat: QA assured
dionisio-bot
bot
added
stat: ready to merge
This reverts commit 73df21d.
coderabbitai
bot
added
type: bug
type: chore
and removed
type: feature
coderabbitai
bot
added
type: feature
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/actions/build-docker/action.yml (2)
86-88: Redundant export statement.
DENO_VERSIONis already set as an environment variable by theenv:block (line 76). Variables defined in theenv:block are automatically available to the shell and child processes, making this export unnecessary.Proposed fix
run: | set -o xtrace - export DENO_VERSION="$DENO_VERSION"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/build-docker/action.yml around lines 86 - 88, Remove the redundant export of DENO_VERSION in the run block: the env-provided DENO_VERSION is already available to the shell, so delete the line `export DENO_VERSION="$DENO_VERSION"` from the run step (the block containing `set -o xtrace`) to avoid unnecessary duplication and keep the action YAML clean.
157-173: Inconsistent interpolation style with the refactored step above.The "Build Docker images" step was refactored to use an
env:block with shell variables, but this step still uses direct${{ inputs.* }}interpolation. While functionally correct for these controlled values, aligning the approach would improve consistency and maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/build-docker/action.yml around lines 157 - 173, The "Save Docker image as artifact" step uses direct GitHub Actions expression interpolation (${ { inputs.* } }) inside the shell script, which is inconsistent with the refactored "Build Docker images" step that uses an env: block and shell variables; change this step to define the inputs as environment variables (e.g., SERVICE, ARCH, TYPE) via an env: block and then reference them in the script with shell variables ($SERVICE, $ARCH, $TYPE), and also use the same pattern for IMAGE (e.g., IMAGE=$(docker compose -f docker-compose-ci.yml config --format json 2>/dev/null | jq -r --arg s "$SERVICE" '.services[$s].image')) so all interpolation is consistent with the refactor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/actions/build-docker/action.yml:
- Around line 86-88: Remove the redundant export of DENO_VERSION in the run
block: the env-provided DENO_VERSION is already available to the shell, so
delete the line `export DENO_VERSION="$DENO_VERSION"` from the run step (the
block containing `set -o xtrace`) to avoid unnecessary duplication and keep the
action YAML clean.
- Around line 157-173: The "Save Docker image as artifact" step uses direct
GitHub Actions expression interpolation (${ { inputs.* } }) inside the shell
script, which is inconsistent with the refactored "Build Docker images" step
that uses an env: block and shell variables; change this step to define the
inputs as environment variables (e.g., SERVICE, ARCH, TYPE) via an env: block
and then reference them in the script with shell variables ($SERVICE, $ARCH,
$TYPE), and also use the same pattern for IMAGE (e.g., IMAGE=$(docker compose -f
docker-compose-ci.yml config --format json 2>/dev/null | jq -r --arg s
"$SERVICE" '.services[$s].image')) so all interpolation is consistent with the
refactor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ffc9e4be-1f82-42ce-8d92-1a1bfee133fb
📒 Files selected for processing (1)
.github/actions/build-docker/action.yml
📜 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: actionlint
- GitHub Check: CodeQL-Build
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
.github/actions/build-docker/action.yml (3)
1-32: LGTM on metadata and input definitions.The added
descriptionfields satisfy actionlint requirements, and thesetup-dockerdefault is now consistently a string'true', aligning with the condition comparison at line 62.
61-63: Condition correctly updated to string comparison.The fix aligns the condition with the string default and is consistent with other input comparisons (e.g.,
inputs.publish-image == 'true'). Callers passing booleanfalsewill correctly skip this step.
92-155: Well-refactored shell logic.Using the env block with properly quoted shell variables avoids potential shell injection issues from direct
${{ }}interpolation. The variable quoting is correct throughout, and thejq --argpattern safely handles service names.
coderabbitai
bot
removed
the
type: feature
1 participant
This PR introduces strict static analysis for our GitHub Actions workflows using
actionlintand resolves all existing syntax andshellcheckviolations.During the cleanup, a critical silent bug was discovered and fixed in the E2E testing workflows where failing tests would bypass Docker cleanup steps, potentially leaving zombie containers on the runners. Additionally, the local
release-actionfails a check because thedist/index.jsfile isn't present before build, so this particular check is ignored in the config file.🛠️ Key Changes
1. Added Actionlint
.github/workflows/actionlint.ymlto automatically lint workflow files onpushandpull_request.2. Fixed Zombie Container Bug in E2E Tests (
ci-test-e2e.yml)npm run testapifailed, Bash'sset -ewould instantly terminate the script, skipping thedocker compose stopcleanup command.npm run testapi || s=$?) to absorb the failure, guarantee the Docker cleanup commands execute, and properly exit with the captured status code (exit "${s:-0}").3. Resolved Shellcheck & Workflow Syntax Errors
SC2086(word splitting) warnings inci.yml,ci-test-e2e.yml, andci-code-check.ymlby wrapping variables in double quotes.SC2016(expressions don't expand in single quotes) by adding a# shellcheck disable=SC2016directive for intentionaljqsingle-quote usage inci.yml.typekeys from input definitions in.github/actions/meteor-build/action.ymland.github/actions/build-docker/action.yml.NPM_TOKENto thesecretscontext inci-test-storybook.ymlandci-test-unit.yml.descriptionkey to.github/actions/setup-playwright/action.yml.descriptionkey at the root ofdedupe-issues.yml.🧪 How to test
Review the CI checks on this PR. The new
actionlintjob should pass cleanly, and the E2E tests should properly spin up, execute, and tear down the Docker containers regardless of test success/failure.Task: FIPS-19
Summary by CodeRabbit
New Features
Bug Fixes
Chores