chore(e2e): Trigger server creating using matterwick for e2e tests#3691
Merged
yasserfaraazkhan merged 22 commits intomasterfrom Mar 18, 2026
Merged
chore(e2e): Trigger server creating using matterwick for e2e tests#3691yasserfaraazkhan merged 22 commits intomasterfrom
yasserfaraazkhan merged 22 commits intomasterfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a cleanup job to the e2e workflow to remove the E2E/Run label from PRs after the workflow completes, intended for runs triggered via Matterwick.
Changes:
- Adds a new
remove-e2e-labeljob that runs afterupdate-final-status - Uses
actions/github-scriptto find an associated PR for the workflow run and remove theE2E/Runlabel
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add run_type input to allow callers to specify PR/RELEASE/MASTER - Add pr_number input for automatic E2E label removal after tests - Add remove-e2e-label job that removes E2E/Run label on completion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The /cleanup_e2e endpoint no longer requires X-Cleanup-Token auth. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This comment was marked as spam.
This comment was marked as spam.
E2E Test Servers CreatedThe following test servers have been created and are ready for E2E testing:
Tests will run against these servers. Please monitor the workflow run for progress. |
All 6 entries in the darwin and linux lists were dead: the popup suite is already CI-skipped via shouldTest(!isCI()), and every linux entry tested a test that is already guarded with `if (process.platform !== 'linux')`. Fix the two legitimately flaky win32 tests instead: - MM-T1304/MM-T1306: raise mocha timeout 30s → 120s (getApp() alone can take up to 120s on Windows runners) - MM-T816: replace asyncSleep(2000) with waitForFunction(isFullScreen) so the assertion is deterministic regardless of runner speed With nothing left to filter, remove known_flaky_tests.json and strip the filtering logic from analyze-flaky-test.js. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Master push E2E triggers are being removed from Matterwick, so the desktop repo no longer needs MASTER as a run type. Nightly runs provide sufficient coverage for the master branch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add permissions: issues: write to remove-e2e-label job - Use SHA matching when selecting from multiple PRs - Improve error messages to not assume 404 means label-only - Add context to eslint-disable directive
![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: 2
🧹 Nitpick comments (2)
e2e/utils/analyze-flaky-test.js (1)
12-22: Simplification looks good; consider renaming for clarity.The removal of flaky test filtering aligns with the PR objectives. However, since the function no longer filters against known flaky tests:
- The variable
newFailedTestsis now misleading—it contains all failed tests, not just "new" ones.- The function name
analyzeFlakyTestsno longer reflects its purpose (it just collects failures).- The error return
{}doesn't match the success return shape{newFailedTests, os}, which could cause downstream issues if callers expect those properties.♻️ Optional: Improve naming and error handling consistency
-function analyzeFlakyTests() { +function getFailedTests() { const os = process.platform; try { const jsonReport = readJsonFromFile(path.join(MOCHAWESOME_REPORT_DIR, 'mochawesome.json')); const {failedFullTitles} = generateShortSummary(jsonReport); - return {newFailedTests: failedFullTitles, os}; + return {failedTests: failedFullTitles, os}; } catch (error) { console.error('Error analyzing failures:', error); - return {}; + return {failedTests: [], os}; } }Note: If you rename the function or return property, ensure callers are updated accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/utils/analyze-flaky-test.js` around lines 12 - 22, The function analyzeFlakyTests no longer filters flaky tests and the name/return shape are misleading: rename the function (e.g., collectFailedTests or getFailedTests) and rename the returned property newFailedTests to something accurate like failedTests; update references to generateShortSummary and the call sites accordingly. Also make the error path return the same shape (e.g., {failedTests: [], os}) instead of {} so callers always receive the same properties; keep use of MOCHAWESOME_REPORT_DIR and readJsonFromFile unchanged. Ensure callers that used analyzeFlakyTests/newFailedTests are updated to the new names..github/workflows/e2e-functional.yml (1)
111-135: Please keep a single label-removal implementation.This job reimplements
removeE2ELabelfrome2e/utils/github-actions.js, and the two paths already diverge on PR discovery and error handling. Either call the shared helper here after checking out the repo, or remove the helper so future fixes only have one place to land.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-functional.yml around lines 111 - 135, The workflow duplicates PR label removal logic in the remove-e2e-label job instead of reusing the shared removeE2ELabel helper from e2e/utils/github-actions.js; update the job to use the single shared implementation by either (A) invoking the existing helper after checking out the repo (add an actions/checkout step and run a small Node/JS step that imports and calls removeE2ELabel with the PR number and github token), or (B) delete the helper and keep this job’s implementation so there is only one place to maintain; reference the job name remove-e2e-label and the helper function removeE2ELabel in e2e/utils/github-actions.js when making the change.
🤖 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/workflows/e2e-functional-template.yml:
- Around line 187-189: The push case only sets BUILD_SUFFIX which leaves TYPE
and ZEPHYR_ENABLE unset and breaks report/Zephyr logic; update the "push" branch
to also set TYPE (e.g., TYPE=build or the same value used by other non-nightly
branches) and set ZEPHYR_ENABLE (e.g., ZEPHYR_ENABLE=false for non-nightly
pushes) so e2e/utils/report.js and e2e/save_report.js receive the expected
values; mirror the pattern used in the other branches for setting TYPE and
ZEPHYR_ENABLE alongside BUILD_SUFFIX to restore correct classification and
Zephyr uploads.
- Around line 37-41: The workflow declares an unused public input named TYPE;
remove the TYPE input declaration (the "TYPE:" block) from the workflow inputs
to avoid confusion, or alternatively modify the e2e/set-required-variables step
to read and honor inputs.TYPE (falling back to the existing event-derived logic)
so callers like e2e-functional.yml can actually control report classification;
specifically target the inputs block where "TYPE" is defined and either delete
that entry or add logic in the e2e/set-required-variables step to use
inputs.TYPE when present.
---
Nitpick comments:
In @.github/workflows/e2e-functional.yml:
- Around line 111-135: The workflow duplicates PR label removal logic in the
remove-e2e-label job instead of reusing the shared removeE2ELabel helper from
e2e/utils/github-actions.js; update the job to use the single shared
implementation by either (A) invoking the existing helper after checking out the
repo (add an actions/checkout step and run a small Node/JS step that imports and
calls removeE2ELabel with the PR number and github token), or (B) delete the
helper and keep this job’s implementation so there is only one place to
maintain; reference the job name remove-e2e-label and the helper function
removeE2ELabel in e2e/utils/github-actions.js when making the change.
In `@e2e/utils/analyze-flaky-test.js`:
- Around line 12-22: The function analyzeFlakyTests no longer filters flaky
tests and the name/return shape are misleading: rename the function (e.g.,
collectFailedTests or getFailedTests) and rename the returned property
newFailedTests to something accurate like failedTests; update references to
generateShortSummary and the call sites accordingly. Also make the error path
return the same shape (e.g., {failedTests: [], os}) instead of {} so callers
always receive the same properties; keep use of MOCHAWESOME_REPORT_DIR and
readJsonFromFile unchanged. Ensure callers that used
analyzeFlakyTests/newFailedTests are updated to the new names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: edd44003-cde2-48c5-8851-2887aa334f0d
📒 Files selected for processing (11)
.github/workflows/compatibility-matrix-testing.yml.github/workflows/e2e-functional-template.yml.github/workflows/e2e-functional.ymle2e/save_report.jse2e/specs/deep_linking/deeplink.test.jse2e/specs/menu_bar/full_screen.test.jse2e/utils/analyze-flaky-test.jse2e/utils/github-actions.jse2e/utils/known_flaky_tests.jsone2e/utils/report.jse2e/utils/test_cases.js
💤 Files with no reviewable changes (3)
- e2e/utils/test_cases.js
- e2e/utils/known_flaky_tests.json
- e2e/utils/report.js
devinbinnie
approved these changes
Mar 16, 2026
cursor Bot
pushed a commit
that referenced
this pull request
Mar 18, 2026
Address CodeRabbit review comments on PR #3691: 1. The inputs.TYPE declaration was unused — the e2e/set-required-variables step derived TYPE from event/flag logic and never read the input. Now the push case honors inputs.TYPE (default: 'PR'), making the input actually functional. 2. The push case only set BUILD_SUFFIX, leaving TYPE and ZEPHYR_ENABLE unset. This caused report.js to fall back to a generic title and save_report.js to skip Zephyr uploads. Now TYPE is set from inputs.TYPE and ZEPHYR_ENABLE is explicitly set to false for push events (the nightly override still correctly sets ZEPHYR_ENABLE=true when applicable). Co-authored-by: yasser khan <attitude3cena.yf@gmail.com>
1 task
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
src/common/servers/serverManager.ts (1)
55-57: Consider defensive handling for missing server entries.The non-null assertion
!assumes all IDs inserverOrderhave corresponding entries inservers. While this should be true under normal operation, a defensive check would prevent crashes if the state becomes inconsistent.💡 Optional: Add defensive filter
getOrderedServers = () => { - return this.serverOrder.map((id) => this.servers.get(id)!); + return this.serverOrder.map((id) => this.servers.get(id)).filter((server): server is MattermostServer => server !== undefined); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/servers/serverManager.ts` around lines 55 - 57, The getOrderedServers method uses a non-null assertion when mapping serverOrder to servers which can throw if an ID is missing; update getOrderedServers to defensively handle missing entries by filtering out undefined lookups from this.servers.get(id) (or substituting a placeholder) before returning the array so that serverOrder and servers inconsistencies don’t crash the process—refer to getOrderedServers, serverOrder and servers to locate and fix the mapping logic.src/common/config/index.ts (1)
119-119: Verify the viewLimit logic handles edge cases correctly.The ternary expression
this.localConfigData?.viewLimit ? Math.max(...) : undefinedwill evaluate toundefinedwhenviewLimitis0(falsy), though0would be an unusual but technically valid value. IfviewLimit: 0should be treated as a valid setting, consider using an explicit!== undefinedcheck instead.💡 Optional: More explicit undefined check
- viewLimit: this.localConfigData?.viewLimit ? Math.max(this.localConfigData.viewLimit, servers.length) : undefined, + viewLimit: this.localConfigData?.viewLimit !== undefined ? Math.max(this.localConfigData.viewLimit, servers.length) : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/config/index.ts` at line 119, The viewLimit assignment uses a falsy check that treats 0 as undefined; update the logic in the config construction (the viewLimit expression referencing this.localConfigData?.viewLimit and servers.length) to explicitly test for undefined (e.g., this.localConfigData?.viewLimit !== undefined) before applying Math.max so that a configured 0 is preserved as a valid value rather than being coerced to undefined.src/app/views/webContentEvents.test.js (1)
57-59: Add a directmattermost://regression case.Lines 57-59 stub the app protocol, but the expanded matrix still only asserts third-party schemes. A refactor that accidentally sends first-party deep links through
allowProtocolDialogorshell.openExternalwould currently slip through this suite.Based on learnings,
WebContentsEventManagercentralizes navigation guards for security and UX control.e2e/specs/mattermost/external_links.test.js (1)
45-141: Use skipped tests here instead of making Linux a silent no-op.The runtime
ifdrops both cases from the suite on Linux, so the Linux job goes green with zero coverage rather than reporting an explicit skip. For a cross-platform E2E signal, that’s easy to misread.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/specs/mattermost/external_links.test.js` around lines 45 - 141, The tests are being silently dropped on Linux due to the runtime "if (process.platform !== 'linux')" which makes the suite show zero tests instead of an explicit skip; change the control so the two tests with titles MM-T_EL_1 and MM-T_EL_2 are explicitly skipped on Linux (e.g., use conditional test skipping like (process.platform === 'linux' ? it.skip : it)(...) or wrap them with describe.skip when process.platform === 'linux') rather than omitting them, keeping the existing test bodies and the shell.openExternal spy and assertions intact so Linux runs report an explicit skipped status.e2e/specs/policy/policy.test.js (1)
78-84: Consider additional escaping for PowerShell metacharacters in server names.The escaping only handles single quotes (
'→''), but server names or URLs containing backticks, dollar signs, or other PowerShell metacharacters could still cause issues when interpolated into the script string before encoding. SincepsEncodereceives the already-interpolated script, malicious or unusual input could still break the command.However, this is a low-risk concern for controlled E2E test data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/specs/policy/policy.test.js` around lines 78 - 84, The current JS-only escaping for single quotes (safeName/safeUrl) is insufficient for some PowerShell metacharacters; update the sanitization before embedding into the PowerShell script so names/URLs also escape backticks and dollar signs (and any other problematic metacharacters you observe) — e.g., extend the replace calls that produce safeName and safeUrl to also replace ` with `` or with the PowerShell-safe escape sequence and to escape $ (e.g., '`$' or doubling strategy consistent with your quoting), or alternatively avoid interpolation by passing the data into PowerShell via ConvertTo-Json / stdin and reconstructing registry entries inside the PS script; modify the code around safeName/safeUrl and the run(...) invocation (and consider psEncode usage) so the final PowerShell command cannot be broken by backticks or $ in input.
🤖 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/workflows/e2e-functional.yml:
- Around line 111-118: The cleanup job remove-e2e-label currently only needs
e2e-tests and uses always(), which lets it run before e2e-policy-tests finishes
or even when the matrix preparation skipped E2E; update the job to depend on
both e2e-tests and e2e-policy-tests by adding e2e-policy-tests to needs and
remove the always() condition so remove-e2e-label will wait for both E2E jobs to
complete and won’t run when prepare-matrix causes the E2E jobs to be skipped
(keep the inputs.pr_number != '' check).
In `@CLAUDE.md`:
- Around line 25-56: Add the appropriate language identifiers to the fenced code
blocks in CLAUDE.md (the block shown around lines 25-56 and the other block
around 150-153) so markdownlint MD040 warnings are resolved; locate the
triple-backtick fences in those sections and change them from ``` to ```text or
```txt (or a more specific language if applicable) ensuring both opening fences
include the language tag.
In `@i18n/de.json`:
- Line 304: Update the translation for the key
renderer.components.settingsPage.checkSpelling.specifyURL to correct the casing
of "URL" and improve the German phrasing; replace the current string with a
clearer sentence such as: "Geben Sie die URL an, von der die
Wörterbuchdefinitionen geladen/abgerufen werden." Ensure the change only updates
the value for that key.
In `@i18n/ru.json`:
- Around line 190-192: The three translation entries
(renderer.components.configureServer.error.basicAuthRequired,
renderer.components.configureServer.error.clientCertRequired,
renderer.components.configureServer.error.preAuthRequired) use informal
"ты/тебе" and must be changed to the formal second person ("Вы/Вам") to match
the rest of the Russian locale; update the phrasing accordingly (e.g., "Вам
будет предложено…" and adjust verb forms/capitalization) and apply the same
normalization to the other affected ranges referenced (lines 208-222, 287-348)
so all server/configuration messages consistently use the formal form.
- Line 69: The translation for the key main.menus.app.edit.pasteAndMatchStyle
currently reads as preserving source formatting but should indicate matching the
destination/style of the surrounding content; update the value for
"main.menus.app.edit.pasteAndMatchStyle" to a Russian phrase that conveys "paste
and match destination style" (for example: "Вставить с оформлением по месту
(соответствует стилю окружения)" or a similarly concise wording that clearly
means matching the destination style).
- Line 404: The translation for the i18n key
renderer.modals.certificate.certificateModal.subject uses "Тема" (meaning
topic); change it to a certificate-appropriate term such as "Субъект" or
"Субъект сертификата" so the X.509 dialog correctly denotes the certificate
subject; update the value for
renderer.modals.certificate.certificateModal.subject accordingly.
In `@src/app/views/webContentEventsCommon.test.ts`:
- Around line 12-15: Update the misleading test comment so it reflects what is
actually being mocked: change the description that mentions
"electron-builder.json protocols" to something like "mock common/constants
MATTERMOST_PROTOCOL" near the jest.mock('common/constants') call; ensure the
comment references MATTERMOST_PROTOCOL so future readers understand the mock
target and purpose in webContentEventsCommon.test.ts.
In `@src/common/views/MattermostView.ts`:
- Around line 57-63: The concatenation logic for building the URL in
MattermostView incorrectly removes only a leading slash when the server path is
root, causing subpath servers to produce URLs like
"https://host/subpathchannels/..."; update the normalization to ensure exactly
one separator between server.url.pathname and this.initialPath by trimming any
trailing slash from server.url.pathname and any leading slash from initialPath
before joining, then pass the joined string into parseURL; update the code that
references initialPath, server.url.pathname, and parseURL in the MattermostView
constructor (or where the URL is built) and add a regression test that
constructs a MattermostView with a server that has a subpath and an initialPath
without a leading slash (e.g., 'channels/town-square') to assert the resulting
URL includes '/subpath/channels/town-square'.
In `@src/renderer/CLAUDE.md`:
- Around line 22-28: The fenced code block in src/renderer/CLAUDE.md is missing
a language tag which triggers markdownlint MD040; update the backticks for the
snippet that starts with "components/" to include a language tag (e.g., change
``` to ```text) so the block is explicitly marked as plain text and the linter
will accept it.
---
Nitpick comments:
In `@e2e/specs/mattermost/external_links.test.js`:
- Around line 45-141: The tests are being silently dropped on Linux due to the
runtime "if (process.platform !== 'linux')" which makes the suite show zero
tests instead of an explicit skip; change the control so the two tests with
titles MM-T_EL_1 and MM-T_EL_2 are explicitly skipped on Linux (e.g., use
conditional test skipping like (process.platform === 'linux' ? it.skip :
it)(...) or wrap them with describe.skip when process.platform === 'linux')
rather than omitting them, keeping the existing test bodies and the
shell.openExternal spy and assertions intact so Linux runs report an explicit
skipped status.
In `@e2e/specs/policy/policy.test.js`:
- Around line 78-84: The current JS-only escaping for single quotes
(safeName/safeUrl) is insufficient for some PowerShell metacharacters; update
the sanitization before embedding into the PowerShell script so names/URLs also
escape backticks and dollar signs (and any other problematic metacharacters you
observe) — e.g., extend the replace calls that produce safeName and safeUrl to
also replace ` with `` or with the PowerShell-safe escape sequence and to escape
$ (e.g., '`$' or doubling strategy consistent with your quoting), or
alternatively avoid interpolation by passing the data into PowerShell via
ConvertTo-Json / stdin and reconstructing registry entries inside the PS script;
modify the code around safeName/safeUrl and the run(...) invocation (and
consider psEncode usage) so the final PowerShell command cannot be broken by
backticks or $ in input.
In `@src/common/config/index.ts`:
- Line 119: The viewLimit assignment uses a falsy check that treats 0 as
undefined; update the logic in the config construction (the viewLimit expression
referencing this.localConfigData?.viewLimit and servers.length) to explicitly
test for undefined (e.g., this.localConfigData?.viewLimit !== undefined) before
applying Math.max so that a configured 0 is preserved as a valid value rather
than being coerced to undefined.
In `@src/common/servers/serverManager.ts`:
- Around line 55-57: The getOrderedServers method uses a non-null assertion when
mapping serverOrder to servers which can throw if an ID is missing; update
getOrderedServers to defensively handle missing entries by filtering out
undefined lookups from this.servers.get(id) (or substituting a placeholder)
before returning the array so that serverOrder and servers inconsistencies don’t
crash the process—refer to getOrderedServers, serverOrder and servers to locate
and fix the mapping logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 501f6f0b-ba1f-4aa3-a437-8e42e3654d73
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (75)
.eslintrc.json.github/actions/install-os-dependencies/action.yaml.github/workflows/build-for-pr.yml.github/workflows/ci.yaml.github/workflows/compatibility-matrix-testing.yml.github/workflows/e2e-functional-template.yml.github/workflows/e2e-functional.yml.github/workflows/nightly-main.yml.github/workflows/nightly-rainforest.yml.github/workflows/release-mas.yaml.github/workflows/release.yaml.github/workflows/scorecards-analysis.ymlCLAUDE.mdNOTICE.txte2e/modules/environment.jse2e/package.jsone2e/specs/mattermost/external_links.test.jse2e/specs/policy/policy.test.jselectron-builder.jsonelectron-builder.tsi18n/de.jsoni18n/en.jsoni18n/nb_NO.jsoni18n/nl.jsoni18n/pl.jsoni18n/ru.jsoni18n/sv.jsonpackage.jsonscripts/patch_mas_version.shsrc/app/CLAUDE.mdsrc/app/mainWindow/serverDropdownView.tssrc/app/navigationManager.test.jssrc/app/navigationManager.tssrc/app/preload/CLAUDE.mdsrc/app/preload/internalAPI.jssrc/app/serverHub.test.jssrc/app/serverHub.tssrc/app/views/CLAUDE.mdsrc/app/views/loadingScreen.test.jssrc/app/views/loadingScreen.tssrc/app/views/pluginsPopUps.test.jssrc/app/views/pluginsPopUps.tssrc/app/views/webContentEvents.test.jssrc/app/views/webContentEvents.tssrc/app/views/webContentEventsCommon.test.tssrc/app/views/webContentEventsCommon.tssrc/common/CLAUDE.mdsrc/common/Validator.tssrc/common/config/CLAUDE.mdsrc/common/config/index.test.jssrc/common/config/index.tssrc/common/constants.tssrc/common/servers/serverManager.test.jssrc/common/servers/serverManager.tssrc/common/utils/url.test.jssrc/common/utils/url.tssrc/common/views/MattermostView.test.jssrc/common/views/MattermostView.tssrc/main/CLAUDE.mdsrc/main/ParseArgs.tssrc/main/app/initialize.test.jssrc/main/app/initialize.tssrc/main/app/utils.tssrc/main/diagnostics/CLAUDE.mdsrc/main/notifications/CLAUDE.mdsrc/main/security/CLAUDE.mdsrc/main/security/allowProtocolDialog.test.jssrc/main/security/allowProtocolDialog.tssrc/main/security/preAuthManager.test.jssrc/main/security/preAuthManager.tssrc/renderer/CLAUDE.mdsrc/renderer/dropdown.tsxsrc/renderer/utils.tssrc/types/config.tssrc/types/window.ts
💤 Files with no reviewable changes (7)
- src/app/preload/internalAPI.js
- .github/actions/install-os-dependencies/action.yaml
- src/main/security/preAuthManager.ts
- electron-builder.json
- src/app/mainWindow/serverDropdownView.ts
- scripts/patch_mas_version.sh
- src/types/window.ts
✅ Files skipped from review due to trivial changes (7)
- src/main/notifications/CLAUDE.md
- src/common/config/CLAUDE.md
- src/app/CLAUDE.md
- src/main/security/CLAUDE.md
- src/main/diagnostics/CLAUDE.md
- src/app/preload/CLAUDE.md
- src/common/CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/compatibility-matrix-testing.yml
yasserfaraazkhan
force-pushed
the
create-e2e-instance-from-matterwick
branch
from
3116c1f to
69f4c87
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/e2e-functional.yml (1)
111-118:⚠️ Potential issue | 🟠 MajorWait for the whole E2E workflow before clearing
E2E/Run.Line 118 can remove the label while
e2e-policy-testsorupdate-final-statusare still running, andalways()also lets this fire whene2e-testswas skipped after matrix prep failed. That makes the PR look done before this workflow actually is.Suggested fix
remove-e2e-label: name: Remove E2E label from PR runs-on: ubuntu-22.04 permissions: issues: write needs: - e2e-tests - if: always() && inputs.pr_number != '' + - e2e-policy-tests + - update-final-status + if: | + always() && + inputs.pr_number != '' && + needs.e2e-tests.result != 'skipped' && + needs.e2e-tests.result != 'cancelled'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-functional.yml around lines 111 - 118, The remove-e2e-label job runs too early; update its needs to wait for the entire E2E workflow before removing the label by adding the remaining end-of-workflow jobs (e.g., e2e-policy-tests and update-final-status) to its needs list so remove-e2e-label depends on e2e-tests, e2e-policy-tests, and update-final-status (keep the existing if condition for inputs.pr_number as appropriate).
🤖 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/workflows/compatibility-matrix-testing.yml:
- Around line 13-17: Validate and sanitize the cmt_run_id workflow input before
embedding it into the JSON payload in the Matterwick cleanup callback: ensure
cmt_run_id contains only digits (or convert/normalize it) and escape/quote it so
the generated JSON is always valid; remove the silent failure pattern that uses
"|| echo" and instead propagate or explicitly log and fail on errors from the
cleanup command so the workflow can surface and handle failures; update the
cleanup step to reject non-numeric cmt_run_id values (or fail fast with a clear
error message) and to return a non-zero exit code when the cleanup request
fails.
In @.github/workflows/e2e-functional.yml:
- Around line 31-35: The workflow currently determines TYPE from github.ref_name
which reflects the dispatching branch, so change the logic that sets TYPE (and
any related env assignment that inspects github.ref_name) to use the workflow
input inputs.version_name (DESKTOP_VERSION) instead; update any conditional that
maps PR/RELEASE (the code referencing github.ref_name and TYPE) to examine
inputs.version_name (or DESKTOP_VERSION) and set TYPE accordingly, and apply the
same replacement for the other occurrence noted in the file.
---
Duplicate comments:
In @.github/workflows/e2e-functional.yml:
- Around line 111-118: The remove-e2e-label job runs too early; update its needs
to wait for the entire E2E workflow before removing the label by adding the
remaining end-of-workflow jobs (e.g., e2e-policy-tests and update-final-status)
to its needs list so remove-e2e-label depends on e2e-tests, e2e-policy-tests,
and update-final-status (keep the existing if condition for inputs.pr_number as
appropriate).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aa77a663-9a76-456c-81b8-c2446daa2698
📒 Files selected for processing (3)
.github/workflows/compatibility-matrix-testing.yml.github/workflows/e2e-functional-template.yml.github/workflows/e2e-functional.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/e2e-functional-template.yml
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/compatibility-matrix-testing.yml (1)
144-165: Validation properly addresses the input injection concern.The numeric regex check at lines 157-160 ensures
CMT_RUN_IDis safe before interpolation, and the--fail-with-bodyflag surfaces HTTP errors.Optional improvement for network resilience.
The
-sflag silences both progress and error output, which could hide debugging information on failure. Adding timeouts and retries would improve reliability for transient network issues.♻️ Optional: Add error visibility and retry handling
- curl -s -X POST "${MATTERWICK_URL}/cleanup_e2e" \ + curl --silent --show-error \ + --connect-timeout 10 \ + --max-time 60 \ + --retry 2 \ + --retry-all-errors \ + -X POST "${MATTERWICK_URL}/cleanup_e2e" \ -H "Content-Type: application/json" \ -H "X-Cleanup-Token: ${CLEANUP_TOKEN}" \ -d "{\"repo\": \"desktop\", \"run_id\": ${CMT_RUN_ID}}" \ --fail-with-body🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/compatibility-matrix-testing.yml around lines 144 - 165, The curl invocation in the cleanup-cmt-instances step currently uses -s which silences error output and lacks retries/timeouts; update the curl command (the one that posts to "${MATTERWICK_URL}/cleanup_e2e" using CMT_RUN_ID and CLEANUP_TOKEN) to make failures visible and resilient by removing -s (or replace with -S), and add sensible curl options like --retry 3 --retry-delay 5 --retry-connrefused --connect-timeout 10 --max-time 30 so transient network errors are retried and timeouts are bounded while still preserving --fail-with-body to surface HTTP error bodies.
🤖 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/workflows/compatibility-matrix-testing.yml:
- Around line 144-165: The curl invocation in the cleanup-cmt-instances step
currently uses -s which silences error output and lacks retries/timeouts; update
the curl command (the one that posts to "${MATTERWICK_URL}/cleanup_e2e" using
CMT_RUN_ID and CLEANUP_TOKEN) to make failures visible and resilient by removing
-s (or replace with -S), and add sensible curl options like --retry 3
--retry-delay 5 --retry-connrefused --connect-timeout 10 --max-time 30 so
transient network errors are retried and timeouts are bounded while still
preserving --fail-with-body to surface HTTP error bodies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 61eb9ae6-8316-4e6b-8d58-39af7d53a29b
📒 Files selected for processing (4)
.github/workflows/cmt-provisioner.yml.github/workflows/compatibility-matrix-testing.yml.github/workflows/e2e-functional.yml.github/workflows/e2e-nightly-trigger.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/e2e-functional.yml
|
This pull request introduces a critical shell injection risk: the workflow .github/workflows/e2e-functional-template.yml directly interpolates the workflow input
🔴 Shell Injection in
|
| Vulnerability | Shell Injection |
|---|---|
| Description | The workflow e2e-functional-template.yml uses ${{ inputs.TYPE }} within a bash script in a way that allows shell metacharacter injection. While inputs.TYPE is technically a workflow input that requires workflow_dispatch trigger (which typically limits who can run it, often to repository maintainers or authorized users), it is treated as trusted input and directly interpolated into a bash script in the e2e/set-required-variables step. Because it is possible for a user to trigger e2e-functional.yml (and thus e2e-functional-template.yml) with arbitrary values for TYPE through workflow_dispatch or via other workflows, this allows a malicious actor to inject shell commands. |
desktop/.github/workflows/e2e-functional-template.yml
Lines 167 to 170 in 77268c4
Comment to provide feedback on these findings.
Report false positive: @dryrunsecurity fp [FINDING ID] [FEEDBACK]
Report low-impact: @dryrunsecurity nit [FINDING ID] [FEEDBACK]
Example: @dryrunsecurity fp drs_90eda195 This code is not user-facing
All finding details can be found in the DryRun Security Dashboard.
This was referenced Apr 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Integrates Matterwick as the E2E test orchestrator for the desktop app. When Matterwick adds the
E2E/Runlabel to a PR, it provisions cloud Mattermost instances and dispatches thee2e-functional.ymlworkflow with instance details and credentials. After tests complete, the label is automatically removed.This PR also adds support for:
compatibility-matrix-testing.ymlwith acmt_run_idand calls back to Matterwick for instance cleanup on completion.The
MASTERrun type has been removed — nightly runs provide sufficient master branch coverage.Ticket Link
SEC-9872
Checklist
E2E/RunScreenshots
N/A — CI/workflow changes only.
Change Impact: 🟡 Medium
Reasoning: The PR shifts E2E orchestration to an external service (Matterwick) and removes the MASTER run type across CI workflows and e2e utilities. Changes are concentrated in CI/workflows and test utilities (no production app logic), but they add external dependencies and new untested control paths, presenting moderate risk.
Regression Risk: The modifications span multiple CI workflows and shared test utility modules (notably e2e/utils/github-actions.js, e2e/utils/report.js, e2e/utils/test_cases.js, and several workflow files). New behaviors include label removal, CMT cleanup callbacks, nightly orchestration, and commit-status updates back to PRs. These introduce untested interactions with GitHub APIs and Matterwick, and removing MASTER alters title/folder mapping logic—risk is moderate given the breadth and external dependencies.
QA Recommendation: Perform focused manual QA covering: (1) Matterwick-triggered flows — provisioning, workflow dispatch, and teardown; (2) removeE2ELabel behavior across PRs (including permission and missing-label cases); (3) CMT cleanup job execution when cmt_run_id is provided and its error handling; (4) nightly runs produce expected TYPE/Zephyr folder mapping and commit status updates per platform; and (5) graceful failure modes when Matterwick is unreachable. Skipping manual QA is not recommended due to new external integrations and lack of unit tests for the added paths.
Generated by CodeRabbitAI