ci(e2e): CMT scheduling + macOS E2E stabilization + CI/security hardening#3829
Open
yasserfaraazkhan wants to merge 36 commits into
Open
ci(e2e): CMT scheduling + macOS E2E stabilization + CI/security hardening#3829yasserfaraazkhan wants to merge 36 commits into
yasserfaraazkhan wants to merge 36 commits into
Conversation
…run webhook GitHub's workflow_run webhook payload does not include workflow_dispatch inputs, so matterwick was never receiving the server_versions value and the CMT Provisioner runs (e.g. 2026-04-28, 2026-05-18) returned 202 OK on the webhook but never dispatched compatibility-matrix-testing.yml. Replace the echo-only stub with a curl POST to matterwick's new /cmt_dispatch endpoint, sending the full context the webhook payload cannot carry: server_versions, run_id, sha, ref, owner, repo. Matterwick returns 202 immediately and runs provisioning asynchronously. Requires two repository configuration items: - vars.MATTERWICK_URL (already set, used by /cleanup_e2e callback) - secrets.MATTERWICK_CMT_TRIGGER_SECRET (new, shared with matterwick) Co-Authored-By: Claude <noreply@anthropic.com>
|
@yasserfaraazkhan: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsI understand the commands that are listed here |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR comprehensively hardens E2E test infrastructure, reporting, and main-window lifecycle management across 77 ranges. It extends per-OS test-count outputs, refactors flaky-test analysis to collapse retries into authoritative outcomes, implements macOS dialog suppression across global setup/teardown/fixtures/workflows, hardens 13 specific tests with polling/event-driven patterns, adds PopoutManager cleanup on main-window close, and refactors intercom readiness with combined show-event and polling-fallback logic. ChangesE2E Infrastructure, Test Reliability, and Main-Window Lifecycle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/cmt-provisioner.yml (1)
21-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet explicit least-privilege
permissionsfor this workflow.No
permissionsblock is defined, so this workflow inherits repository defaults forGITHUB_TOKEN, which is broader than needed here.🔐 Proposed fix
name: CMT Provisioner +permissions: {} on: workflow_dispatch:🤖 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/cmt-provisioner.yml around lines 21 - 31, Add an explicit top-level permissions block to this workflow to avoid inheriting broad repository defaults; update the workflow YAML (where the on:, workflow_dispatch and job trigger-matterwick are defined) to include a minimal permissions map such as setting permissions: contents: read and workflows: write (adjust further only if the trigger-matterwick job needs additional scopes), so the GITHUB_TOKEN only has the least-privilege required.
🤖 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.
Outside diff comments:
In @.github/workflows/cmt-provisioner.yml:
- Around line 21-31: Add an explicit top-level permissions block to this
workflow to avoid inheriting broad repository defaults; update the workflow YAML
(where the on:, workflow_dispatch and job trigger-matterwick are defined) to
include a minimal permissions map such as setting permissions: contents: read
and workflows: write (adjust further only if the trigger-matterwick job needs
additional scopes), so the GITHUB_TOKEN only has the least-privilege required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 33b71075-cd7f-4654-978e-a49a6b70c36d
📒 Files selected for processing (1)
.github/workflows/cmt-provisioner.yml
The workflow inherits the repo default GITHUB_TOKEN permissions, which
is broader than this workflow needs. The single step is an outbound
curl to matterwick authenticated by a separate shared secret -- there
is no checkout, no GitHub API call, and no interaction with workflow
files. permissions: {} (no scopes) is the correct least-privilege
setting; if a future step needs a specific scope, it should be added
explicitly rather than relying on repo-default inheritance.
Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/cmt-provisioner.yml (1)
61-68:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winQuote
${RUN_ID}in the jq command.Line 66 uses
--argjson run_id ${RUN_ID}without quoting${RUN_ID}. Whilegithub.run_idshould always be numeric, unquoted shell expansions can cause issues if the value is empty, contains whitespace, or has special characters. Quote it for safety.🛡️ Proposed fix
- --argjson run_id ${RUN_ID} \ + --argjson run_id "${RUN_ID}" \🤖 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/cmt-provisioner.yml around lines 61 - 68, The jq payload construction is vulnerable to unquoted shell expansion for RUN_ID; update the payload assignment so the jq invocation quotes the expansion for run id (the line using --argjson run_id ${RUN_ID}) to --argjson run_id "${RUN_ID}" (i.e., modify the jq command in the payload assignment to quote ${RUN_ID}) to prevent word-splitting or empty-value issues while keeping the --argjson usage.
🤖 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.
Outside diff comments:
In @.github/workflows/cmt-provisioner.yml:
- Around line 61-68: The jq payload construction is vulnerable to unquoted shell
expansion for RUN_ID; update the payload assignment so the jq invocation quotes
the expansion for run id (the line using --argjson run_id ${RUN_ID}) to
--argjson run_id "${RUN_ID}" (i.e., modify the jq command in the payload
assignment to quote ${RUN_ID}) to prevent word-splitting or empty-value issues
while keeping the --argjson usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 087195d9-68cf-4e38-852e-8395a205a823
📒 Files selected for processing (1)
.github/workflows/cmt-provisioner.yml
Without quotes, an empty RUN_ID (e.g. GHA context expansion failing) would collapse the --argjson token list and silently produce a malformed payload or be consumed by the next --arg. Quoting forces jq to receive an empty string and fail immediately with "invalid JSON text passed to --argjson", which is what we want — fail loudly, don't ship a bad request. Co-Authored-By: Claude <noreply@anthropic.com>
github-actions
Bot
added
E2E/Run
This trigger workflow had both a schedule (Thu/Fri 00:00 UTC) and a
push trigger on master + release-*. The push trigger was redundant
because matterwick's push-event handler also fires for master and
release-* pushes (E2EAutoTriggerOnMaster / E2EAutoTriggerOnRelease in
the deployed config). The result: every push to master or release-*
produced two concurrent matterwick paths -- direct push-event handler
(tracking key {repo}-push-{branch}-{sha}) AND workflow_run on this
trigger workflow (tracking key {repo}-scheduled-{runID}-{sha}) --
which provisioned 6 cloud instances instead of 3 and dispatched
e2e-functional.yml twice per commit.
Tracking keys differ, so the two paths don't collide in matterwick
state, but they race on commit status updates and waste cloud quota.
Leaving only the schedule trigger here makes matterwick's push handler
the single canonical driver for push-triggered E2E.
Co-Authored-By: Claude <noreply@anthropic.com>
github-actions
Bot
added
E2E/Run
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
- Suppress macOS Resume dialog (NSQuitAlwaysKeepsWindows, ApplePersistenceIgnoreState) - Suppress macOS crash reporter dialog (CrashReporter DialogType none) - Suppress Gatekeeper quarantine dialog (LSQuarantine) - Add --no-first-run, --no-default-browser-check, --disable-default-apps, --disable-crash-reporter flags - Apply defaults write at workflow level (belt-and-suspenders for CI) - Avoid SIGKILL on macOS in global-teardown (triggers quit-unexpectedly dialog) - Increase waitForAppReady timeout to 60s on macOS - Exclude retried failures from analyzeFlakyTests when exit code is 0
github-actions
Bot
added
E2E/Run
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
e2e/global-setup.ts (2)
48-49: ⚡ Quick winMisleading comment.
The comment states "Verify at least one bundle ID got the settings applied" but no verification code follows. The subsequent lines (52-63) apply additional settings to system-level domains (LaunchServices, CrashReporter) rather than verifying the bundle-specific settings.
📝 Clarify the comment
- // Verify at least one bundle ID got the settings applied. - // Also suppress the "verification of developer" dialog that can appear + // Suppress the "verification of developer" dialog that can appear🤖 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 `@e2e/global-setup.ts` around lines 48 - 49, The comment claiming "Verify at least one bundle ID got the settings applied" is misleading — update the comment above the subsequent settings block to accurately describe what the code does (apply additional system-level settings and suppress the "verification of developer" dialog) and remove the mention of bundle-ID verification; reference the system domains mentioned (LaunchServices, CrashReporter) in the new comment so it correctly documents that the following lines adjust system-level domains rather than performing any bundle-specific verification.
30-64: 💤 Low valueConsider using execFileSync for security.
The current implementation uses
execSyncwith string interpolation ofbundleID. While the current code uses a hard-coded array, usingexecFileSyncwith argument arrays would be more robust against potential future changes and aligns with security best practices.🔒 Proposed refactor using execFileSync
-import {execSync} from 'child_process'; +import {execFileSync} from 'child_process';for (const bundleID of bundleIDs) { try { - execSync(`defaults write ${bundleID} NSQuitAlwaysKeepsWindows -bool false`, {stdio: 'pipe'}); + execFileSync('defaults', ['write', bundleID, 'NSQuitAlwaysKeepsWindows', '-bool', 'false'], {stdio: 'pipe'}); } catch { // Non-fatal — tests still run, just potentially with the Resume dialog } try { - execSync(`defaults write ${bundleID} ApplePersistenceIgnoreState -bool YES`, {stdio: 'pipe'}); + execFileSync('defaults', ['write', bundleID, 'ApplePersistenceIgnoreState', '-bool', 'YES'], {stdio: 'pipe'}); } catch { // Non-fatal } } try { - execSync('defaults write com.apple.LaunchServices LSQuarantine -bool false', {stdio: 'pipe'}); + execFileSync('defaults', ['write', 'com.apple.LaunchServices', 'LSQuarantine', '-bool', 'false'], {stdio: 'pipe'}); } catch { // Non-fatal } try { - execSync('defaults write com.apple.CrashReporter DialogType none', {stdio: 'pipe'}); + execFileSync('defaults', ['write', 'com.apple.CrashReporter', 'DialogType', 'none'], {stdio: 'pipe'}); } catch { // Non-fatal }🤖 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 `@e2e/global-setup.ts` around lines 30 - 64, Replace the execSync calls in the macOS branch (process.platform === 'darwin') that interpolate bundleID into command strings with execFileSync using argument arrays: locate the loop over bundleIDs and the standalone execSync calls (symbols: bundleIDs, execSync) and call execFileSync('defaults', ['write', bundleID, 'NSQuitAlwaysKeepsWindows', '-bool', 'false'], {stdio:'pipe'}) and similarly for the other keys (ApplePersistenceIgnoreState, LSQuarantine, DialogType) to avoid shell interpolation; preserve the existing try/catch non-fatal behavior and same stdio option.
🤖 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.
Nitpick comments:
In `@e2e/global-setup.ts`:
- Around line 48-49: The comment claiming "Verify at least one bundle ID got the
settings applied" is misleading — update the comment above the subsequent
settings block to accurately describe what the code does (apply additional
system-level settings and suppress the "verification of developer" dialog) and
remove the mention of bundle-ID verification; reference the system domains
mentioned (LaunchServices, CrashReporter) in the new comment so it correctly
documents that the following lines adjust system-level domains rather than
performing any bundle-specific verification.
- Around line 30-64: Replace the execSync calls in the macOS branch
(process.platform === 'darwin') that interpolate bundleID into command strings
with execFileSync using argument arrays: locate the loop over bundleIDs and the
standalone execSync calls (symbols: bundleIDs, execSync) and call
execFileSync('defaults', ['write', bundleID, 'NSQuitAlwaysKeepsWindows',
'-bool', 'false'], {stdio:'pipe'}) and similarly for the other keys
(ApplePersistenceIgnoreState, LSQuarantine, DialogType) to avoid shell
interpolation; preserve the existing try/catch non-fatal behavior and same stdio
option.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 70033d38-66b6-4678-a46a-f5d9fc89d10a
📒 Files selected for processing (7)
.github/workflows/e2e-functional-template.yml.github/workflows/e2e-functional.ymle2e/fixtures/index.tse2e/global-setup.tse2e/global-teardown.tse2e/helpers/appReadiness.tse2e/utils/analyze-flaky-test.js
- Replace execSync with execFileSync using argument arrays to avoid shell interpolation of bundleID values - Fix misleading comment that claimed "verify at least one bundle ID" when the block actually applies system-level LaunchServices/CrashReporter settings, not per-bundle verification
github-actions
Bot
added
E2E/Run
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
.github/workflows/e2e-functional-template.yml (1)
395-410:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the standard release bucket and CDN URL for Playwright reports.
This still hardcodes the backing bucket and emits a direct S3 URL. The repo workflow convention is to upload via
MM_DESKTOP_RELEASE_BUCKETand publishreleases.mattermost.com/desktop/...links instead.💡 Suggested adjustment
- AWS_S3_BUCKET: mattermost-cypress-report + AWS_S3_BUCKET: ${{ secrets.MM_DESKTOP_RELEASE_BUCKET }} ... - REPORT_URL="https://${AWS_S3_BUCKET}.s3.amazonaws.com/${S3_PREFIX}/index.html" + REPORT_URL="https://releases.mattermost.com/desktop/${S3_PREFIX}/index.html"Based on learnings: "Artifact uploads in Mattermost Desktop workflows should funnel to a backend S3 bucket defined by the MM_DESKTOP_RELEASE_BUCKET secret, but the public URL for accessing those artifacts must always be releases.mattermost.com/desktop via the CDN/mapping layer."
🤖 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/e2e-functional-template.yml around lines 395 - 410, Replace the hardcoded AWS_S3_BUCKET and direct S3 REPORT_URL with the repo-standard secret and CDN URL: set AWS_S3_BUCKET to the secret MM_DESKTOP_RELEASE_BUCKET (instead of secrets.MM_DESKTOP_E2E_AWS_SECRET_ACCESS_KEY or the current literal), keep S3_PREFIX as-is, use that bucket in the aws s3 sync target, and construct REPORT_URL using the CDN mapping "https://releases.mattermost.com/desktop/${S3_PREFIX}/index.html" so uploads go to the canonical release bucket and public links use releases.mattermost.com/desktop.
🧹 Nitpick comments (2)
src/main/UserActivityMonitor.test.js (1)
6-15: ⚡ Quick winUse the repo-standard ESM singleton mock shape.
This
electronmock omits__esModule: trueanddefault, so it diverges from the required*.test.jssingleton pattern and can cause import interop drift across the suite.♻️ Suggested adjustment
+const electronMock = { + app: { + isReady: jest.fn(() => true), + }, + powerMonitor: { + getSystemIdleTime: jest.fn(() => 0), + }, +}; + -jest.mock('electron', () => ({ - app: { - isReady: jest.fn(() => true), - }, - powerMonitor: { - getSystemIdleTime: jest.fn(() => 0), - }, -})); +jest.mock('electron', () => ({ + __esModule: true, + default: electronMock, + ...electronMock, +}));As per coding guidelines: "
**/*.test.{js,ts}: Mock singletons withjest.mock()using__esModule: true+defaultproperty, and usejest.mocked()for type-safe access to mock functions`."🤖 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 `@src/main/UserActivityMonitor.test.js` around lines 6 - 15, The electron mock is missing the repo-standard ESM singleton shape; update the jest.mock('electron', ...) to return { __esModule: true, default: { app: { isReady: jest.fn(() => true) }, powerMonitor: { getSystemIdleTime: jest.fn(() => 0) } } } so imports get the default ESM singleton and the mocked functions (app.isReady and powerMonitor.getSystemIdleTime) behave correctly; after changing the mock shape, use jest.mocked(...) when referencing these mocks elsewhere in the test to keep type-safe access.src/main/diagnostics/index.test.js (1)
6-30: ⚡ Quick winUse the repo-standard ESM singleton mock shape here too.
This new
electronmock has the same issue: it skips the required__esModule: true+defaultwrapper for singleton mocks, so it does not match the repo's Jest mock contract.As per coding guidelines: "
**/*.test.{js,ts}: Mock singletons withjest.mock()using__esModule: true+defaultproperty, and usejest.mocked()for type-safe access to mock functions`."🤖 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 `@src/main/diagnostics/index.test.js` around lines 6 - 30, The electron jest.mock should use the repo-standard ESM singleton shape by returning an object with __esModule: true and a default property containing the mocked API; update the existing jest.mock('electron', ...) to wrap the current mock (shell.showItemInFolder, app.isReady/getPath, powerMonitor.getSystemIdleTime, session.defaultSession, systemPreferences.getMediaAccessStatus, Notification) under a default key and add __esModule: true so the singleton mock contract is satisfied; after changing the mock shape, use jest.mocked(...) where this mock is referenced in tests to get type-safe access to the mocked functions.
🤖 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/compatibility-matrix-testing.yml:
- Around line 144-146: The workflow's dispatch input docs for "cmt_run_id" are
stale and still claim instance cleanup is driven by cmt_run_id; update the
`workflow_dispatch` input description for cmt_run_id (and any nearby comment
block) to state that instance teardown is keyed by commit SHA and handled
externally by Matterwick when the workflow_run completes, or remove/rename the
input if it's no longer applicable; ensure the `cmt_run_id` text matches the new
cleanup flow described in the top comment about commit SHA-based cleanup.
In @.github/workflows/e2e-functional-template.yml:
- Around line 167-205: The sync-pr-server-url job currently checks out
inputs.DESKTOP_VERSION and then runs actions/github-script which requires
./e2e/utils/github-actions.js while the job has pull-requests: write — this lets
an untrusted PR ref modify that helper and execute code with write privileges.
Fix by removing the ability for checked-out PR-controlled code to run with write
permissions: either inline the script logic from github-actions.js into the
workflow step (so it does not require code from the checkout) or change the
checkout to a trusted immutable ref (pin to a commit/tag) and/or remove the
pull-requests: write permission from the job; update the
sync-cursor-automation-server-on-pr step to use the inlined script or the pinned
helper instead of require('./e2e/utils/github-actions.js').
In @.github/workflows/e2e-functional.yml:
- Around line 188-192: The e2e-policy-tests job currently grants statuses: write
to all its steps; remove statuses: write from the e2e-policy-tests job's
permissions (keep contents: read) so test execution (npm ci, npm run build-test,
npm run run:policy) is read-only, then add a separate dependent job (e.g.,
e2e-policy-postprocess) that has needs: e2e-policy-tests and permissions
including statuses: write and move the e2e/check-for-failures step into that
post-processing job so only that job can update commit status. Ensure job names
referenced are e2e-policy-tests and e2e-policy-postprocess and the check step
identifier is e2e/check-for-failures.
In `@qa-report.md`:
- Around line 33-36: Update the "CMT provisioner dispatches Matterwick via HTTP"
bullet in qa-report.md to reflect that this PR changes CMT to dispatch
Matterwick using the workflow_run webhook inputs (i.e., driven by workflow_run
events) instead of HTTP; locate the "CMT provisioner" bullet and replace the
transport description to explicitly state "dispatches Matterwick via
workflow_run webhook inputs" or similar phrasing so rollout notes and risk
analysis are accurate.
---
Duplicate comments:
In @.github/workflows/e2e-functional-template.yml:
- Around line 395-410: Replace the hardcoded AWS_S3_BUCKET and direct S3
REPORT_URL with the repo-standard secret and CDN URL: set AWS_S3_BUCKET to the
secret MM_DESKTOP_RELEASE_BUCKET (instead of
secrets.MM_DESKTOP_E2E_AWS_SECRET_ACCESS_KEY or the current literal), keep
S3_PREFIX as-is, use that bucket in the aws s3 sync target, and construct
REPORT_URL using the CDN mapping
"https://releases.mattermost.com/desktop/${S3_PREFIX}/index.html" so uploads go
to the canonical release bucket and public links use
releases.mattermost.com/desktop.
---
Nitpick comments:
In `@src/main/diagnostics/index.test.js`:
- Around line 6-30: The electron jest.mock should use the repo-standard ESM
singleton shape by returning an object with __esModule: true and a default
property containing the mocked API; update the existing jest.mock('electron',
...) to wrap the current mock (shell.showItemInFolder, app.isReady/getPath,
powerMonitor.getSystemIdleTime, session.defaultSession,
systemPreferences.getMediaAccessStatus, Notification) under a default key and
add __esModule: true so the singleton mock contract is satisfied; after changing
the mock shape, use jest.mocked(...) where this mock is referenced in tests to
get type-safe access to the mocked functions.
In `@src/main/UserActivityMonitor.test.js`:
- Around line 6-15: The electron mock is missing the repo-standard ESM singleton
shape; update the jest.mock('electron', ...) to return { __esModule: true,
default: { app: { isReady: jest.fn(() => true) }, powerMonitor: {
getSystemIdleTime: jest.fn(() => 0) } } } so imports get the default ESM
singleton and the mocked functions (app.isReady and
powerMonitor.getSystemIdleTime) behave correctly; after changing the mock shape,
use jest.mocked(...) when referencing these mocks elsewhere in the test to keep
type-safe access.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 05a40bdd-e2a8-4d4d-a3a0-c3d798516dbb
📒 Files selected for processing (8)
.github/workflows/cmt-provisioner.yml.github/workflows/compatibility-matrix-testing.yml.github/workflows/e2e-functional-template.yml.github/workflows/e2e-functional.ymlqa-report.mdsrc/main/UserActivityMonitor.test.jssrc/main/app/intercom.test.jssrc/main/diagnostics/index.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/app/intercom.test.js
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Removes e2e/helpers/resolveMmTestServerUrlFromPr.ts and all related logic: - e2e/global-setup.ts: drop the resolveMmTestServerUrlFromPrIfNeeded() call/import. - e2e/utils/github-actions.js: remove parseCursorAutomationServerUrlFromBody and syncCursorAutomationServerLine (now unused). - .github/workflows/e2e-functional-template.yml: remove the sync-pr-server-url job and the pr_number inputs (only consumed by that job); the e2e test job is unchanged. - .github/workflows/e2e-functional.yml: stop passing pr_number to the template and drop the pull-requests:write caller cap (template no longer writes to PRs); the top-level pr_number input is kept for E2E-label cleanup. - Docs (AGENTS.md, e2e/AGENTS.md): drop the now-inaccurate auto-resolve-from-PR notes; MM_TEST_SERVER_URL must be set explicitly. MM_TEST_SERVER_URL is now provided only via inputs/env (matterwick passes it for provisioned runs); it is no longer derived from the PR body. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
E2E/Run
…s, docs) Follow-on to the resolveMmTestServerUrlFromPr.ts deletion: remove all related logic. - e2e/global-setup.ts: drop resolveMmTestServerUrlFromPrIfNeeded() call + import. - e2e/utils/github-actions.js: remove parseCursorAutomationServerUrlFromBody and syncCursorAutomationServerLine (now unused). - e2e-functional-template.yml: remove the sync-pr-server-url job and the pr_number inputs (only that job consumed them). - e2e-functional.yml: stop passing pr_number to the template and drop the pull-requests:write caller cap (template no longer writes PRs); top-level pr_number kept for E2E-label cleanup. - AGENTS.md / e2e/AGENTS.md: drop the now-inaccurate auto-resolve-from-PR notes. MM_TEST_SERVER_URL is now supplied only via inputs/env, never derived from the PR body. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
E2E/Run
Both AGENTS.md (repo root) and e2e/AGENTS.md now match master — the PR makes no documentation changes. (Root AGENTS.md was already net-zero vs master; this reverts the leftover e2e/AGENTS.md note, which duplicated the env-var list right above it.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
E2E/Run
Cleanup is no longer driven by cmt_run_id — instance teardown is keyed by commit SHA and handled by Matterwick on workflow_run completion. The input stays declared (Matterwick still passes it; an undeclared input would 422 the dispatch). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
E2E/Run
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
yasserfaraazkhan
changed the title
devinbinnie
requested changes
Jun 3, 2026
Member
devinbinnie
left a comment
devinbinnie
left a comment
There was a problem hiding this comment.
Thanks for continuing to work to stabilize the Desktop E2E tests :)
I do have some concerns around changing we need to make to the code base to make this work - especially the polling addition, since that definitely can affect normal app usage.
…atch-and-cleanup-endpoint
github-actions
Bot
added
E2E/Run
Addresses @devinbinnie's review: - popoutManager: reverted the registerMainWindowCloseHandler/closeAllPopouts change (and its tests). Destroying popouts when the main window closes contradicts the intended multi-window independence, and isn't needed — E2E teardown handles cleanup. - intercom.handleMainWindowIsShown: the production onboarding path is back to its original behavior. The E2E readiness signal (__e2eAppReady) moved into a separate signalE2EAppReadyWhenShown() that is gated on NODE_ENV==='test' (so it adds no listeners and is inert in normal app usage) and is purely listener-based — the 250ms isVisible() polling is removed. The already-visible check + once('show') listener + MAIN_WINDOW_CREATED wait cover the readiness cases without polling. Tests updated accordingly (removed the polling-fallback test). intercom, popoutManager, and the electron-mock suites pass (71 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
E2E/Run
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Enables matterwick-driven scheduled/CMT E2E provisioning for Desktop and lands a broad round of E2E reliability and CI/security hardening. (Companion to
mattermost/matterwick#90.)CMT / scheduled provisioning (matterwick-driven)
cmt-provisioner.yml→ lightweight monthly scheduled trigger (no inputs/curl); matterwick hears theworkflow_runand dispatchescompatibility-matrix-testing.yml.compatibility-matrix-testing.yml→ dropped the in-workflow/cleanup_e2ecall (matterwick now destroys provisioned servers onworkflow_runcompletion, keyed by commit SHA); corrected the stalecmt_run_idinput description.e2e-nightly-trigger.yml→ schedule-only (pushes tomain/release-*are handled by matterwick's native push handler).macOS E2E stabilization (the previously-red macOS suite)
intercom.ts: fixed the__e2eAppReadyrace (the windowshowevent firing before the listener attaches, orMainWindow.get()returningundefined, on slower macOS runners) — the single error behind nearly all macOS failures. Added unit tests.SIGKILLin global teardown, and bumped app-ready / minimize timeouts (global-setup.ts,global-teardown.ts,appReadiness.ts).analyze-flaky-test.js,github-actions.js).role="menuitem"selector, window-menu/minimize timeout, deeplink WebContentsView traversal (Windows), bad-servers cert reload, remove-server-modal partial-config parse, badge, full-screen, tray-restore, startup window.CI & security hardening
e2e-functional.ymlleast-privilege permissions (top-levelcontents: read, per-job grants) so untrusted PR code (npm ci/ Playwright) no longer runs withpull-requests: write.electronin the two unit suites (UserActivityMonitor,diagnostics) that imported the real module — they only passed when the electron binary happened to be installed.popoutManager:registerMainWindowCloseHandler/closeAllPopouts+ unit tests.Cursor-automation removal
Removed the "Server for Cursor Automation" PR-body read/write feature (
resolveMmTestServerUrlFromPr, the PR-body sync job, and the helper fns).MM_TEST_SERVER_URLis supplied only via inputs/env.Companion PRs
mattermost/matterwick#90,mattermost/mattermost-mobile#9800,mattermost/gitops-platform#72.🤖 Generated with Claude Code