fix: Fix selector-purity violation and E2E impact analysis in CI by shortstacked · Pull Request #26410 · n8n-io/n8n

shortstacked · 2026-03-02T11:03:28Z

Summary

What started as a small janitor fix uncovered two bugs in the CI impact analysis pipeline. This PR fixes all three issues:

1. Selector-purity violation (canvas-zoom.spec.ts)
Moved a chained .locator('input') call out of the test and into CanvasPage as getRenameInput(), fixing the selector-purity error caught by the janitor.

2. Broken playwright-only fallback in CI
When impact analysis found no affected specs, distribute-tests.mjs emitted a sentinel matrix entry with empty specs, causing Playwright to run all 746 tests on a single shard — far worse than the normal 16-shard run. Fixed by:

Adding skip: true to the no-specs sentinel
Capturing a skip-tests output from the prepare job and using it to skip multi-main-e2e entirely via needs context

3. Impact analysis silently returning no changed files
git diff --name-only origin/master...HEAD (three-dot) requires a merge base, which fails with fatal: no merge base on shallow CI checkouts. The janitor's getChangedFiles was catching the exception and returning [], making every playwright-only run report no affected specs.

The CI workflow now computes the changed file list itself (fetch + FETCH_HEAD HEAD two-dot diff, matching the approach used by ci-filter across the rest of the repo) and passes it directly to the janitor via --files=. This decouples the janitor from git operations in CI entirely — impact analysis is now pure AST. The janitor's --base path is preserved for local use.

Related Linear tickets, Github issues, and Community forum posts

Review / Merge checklist

PR title and summary are descriptive. (conventions)
Docs updated or follow-up ticket created. (internal tooling only)
Tests included. (all changes verified via TCR and CI)
PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

…ector-purity violation

aikido-pr-checks · 2026-03-02T11:03:49Z

packages/testing/playwright/pages/CanvasPage.ts

 		return this.page.locator('.rename-prompt');
 	}

+	getRenameInput(): Locator {


CanvasPage.ts exceeds recommended size; adding getRenameInput increases an already large class. Consider moving related locator helpers into a smaller, focused helper or component file.

Details

✨ AI Reasoning
A small accessor method (getRenameInput) was added, increasing the file's size. Large files that mix many Playwright page helpers and component wrappers are harder to navigate and maintain. Adding additional methods to an already large, single-responsibility Page class further concentrates unrelated surface-area in one file, making future changes and discovery of related helpers more difficult for maintainers.

🔧 How do I fix it?
Split large files into smaller, focused modules. Each file should have a single responsibility.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}
_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

cubic-dev-ai

No issues found across 2 files

Architecture diagram

sequenceDiagram
    participant Test as canvas-zoom.spec.ts
    participant POM as CanvasPage (POM)
    participant Engine as Playwright / Browser

    Note over Test, Engine: E2E Rename Workflow Flow

    Test->>Engine: keyboard.type('Y')
    
    rect rgb(23, 37, 84)
    Note right of Test: Verification Phase
    Test->>POM: CHANGED: getRenameInput()
    POM->>POM: getRenamePrompt()
    POM->>Engine: NEW: locator('.rename-prompt').locator('input')
    Engine-->>POM: Locator Object
    POM-->>Test: Return Locator
    end

    Test->>Engine: expect(locator).toHaveValue('X: Y')
    Engine-->>Test: Assertion Result

    Test->>Engine: keyboard.press('Enter')
    Test->>POM: nodeByName('X: Y')
    POM->>Engine: locator logic
    Engine-->>Test: Element Attached Confirmation

When playwright-only mode detects no affected specs, the fallback was emitting a sentinel matrix entry with empty specs, causing all 746 tests to run on a single shard — far worse than the normal 16-shard run. - Add skip:true to the no-specs sentinel in distribute-tests.mjs - Capture skip-tests output from prepare job in test-e2e-ci-reusable.yml - Skip multi-main-e2e entirely when skip-tests=true (uses needs context, which is valid at job-level if unlike matrix context) - Forward janitor stderr to CI output so impact analysis stats are visible Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…lity The three-dot syntax (origin/master...HEAD) requires finding a merge base, which fails with fatal: no merge base on shallow CI checkouts. This caused getChangedFiles to silently return [], making impact analysis report no changed files even when specs were directly modified. Switch to fetch + FETCH_HEAD two-dot diff, matching the approach used by the ci-filter action across the rest of the repo. Remove the now-redundant 'Fetch base branch' step from test-e2e-ci-reusable.yml since the janitor handles the fetch internally. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

codecov · 2026-03-02T12:00:47Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Instead of having the janitor derive changed files from git internally (fetch + diff), the CI workflow now computes the file list and passes it directly via --files. This decouples the janitor from git operations in CI: - Add 'Get changed files' step in test-e2e-ci-reusable.yml that fetches the base branch and diffs FETCH_HEAD HEAD for the file list - Pass comma-separated file list to distribute-tests.mjs via --files= instead of --base=origin/master - distribute-tests.mjs threads --files= through to the janitor CLI - If the diff returns nothing (git failure), falls back to --impact only and the janitor handles it via its own git fallback The janitor's --base path remains intact for local use. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

If playwright-only=true, ci-filter already confirmed files changed using the same fetch+diff approach, so CHANGED_FILES will never be empty. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

cubic-dev-ai

1 issue found across 2 files (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="packages/testing/playwright/scripts/distribute-tests.mjs">

<violation number="1" location="packages/testing/playwright/scripts/distribute-tests.mjs:49">
P1: Custom agent: **Security Review**

Unsanitized `--files` input is interpolated into an `execSync` shell command, creating a command-injection path. Escape or safely quote the value before composing the command string (or use a non-shell argument API).</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

packages/testing/playwright/scripts/distribute-tests.mjs

Replace execSync (shell string) with execFileSync (args array) to prevent command injection via unsanitized --files input. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

shortstacked · 2026-03-02T12:52:39Z

Fixed in 80b4f99 — switched from execSync (shell string interpolation) to execFileSync (args array). The --files value is now passed as a discrete argument, never interpreted by a shell.

n8n-assistant · 2026-03-03T08:17:15Z

Got released with n8n@2.11.0

fix(playwright): move rename input locator into CanvasPage to fix sel…

351576b

…ector-purity violation

aikido-pr-checks bot reviewed Mar 2, 2026

View reviewed changes

shortstacked changed the title ~~fix(playwright): move rename input locator into CanvasPage to fix sel…~~ fix(playwright): Move rename input locator into CanvasPage Mar 2, 2026

cubic-dev-ai bot reviewed Mar 2, 2026

View reviewed changes

shortstacked changed the title ~~fix(playwright): Move rename input locator into CanvasPage~~ test: Move rename input locator into CanvasPage Mar 2, 2026

n8n-assistant bot added the n8n team Authored by the n8n team label Mar 2, 2026

shortstacked and others added 2 commits March 2, 2026 12:37

refactor(ci): remove dead code guard for empty CHANGED_FILES

04c0fc7

If playwright-only=true, ci-filter already confirmed files changed using the same fetch+diff approach, so CHANGED_FILES will never be empty. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

cubic-dev-ai bot reviewed Mar 2, 2026

View reviewed changes

packages/testing/playwright/scripts/distribute-tests.mjs Outdated Show resolved Hide resolved

shortstacked changed the title ~~test: Move rename input locator into CanvasPage~~ fix(playwright): Fix selector-purity violation and E2E impact analysis in CI Mar 2, 2026

fix(testing): Avoid shell interpolation in distribute-tests script

80b4f99

Replace execSync (shell string) with execFileSync (args array) to prevent command injection via unsanitized --files input. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

shortstacked changed the title ~~fix(playwright): Fix selector-purity violation and E2E impact analysis in CI~~ fix: Fix selector-purity violation and E2E impact analysis in CI Mar 2, 2026

shortstacked requested a review from Matsuuu March 2, 2026 13:42

shortstacked enabled auto-merge March 2, 2026 13:42

Matsuuu approved these changes Mar 2, 2026

View reviewed changes

shortstacked added this pull request to the merge queue Mar 2, 2026

Merged via the queue into master with commit 7be48a4 Mar 2, 2026
44 of 45 checks passed

shortstacked deleted the playwright-janitor-test branch March 2, 2026 15:17

This was referenced Mar 3, 2026

🚀 Release 2.11.0 #26455

Closed

🚀 Release 2.11.0 #26456

Merged

n8n-assistant bot added the Released label Mar 3, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix selector-purity violation and E2E impact analysis in CI#26410

fix: Fix selector-purity violation and E2E impact analysis in CI#26410
shortstacked merged 6 commits intomasterfrom
playwright-janitor-test

shortstacked commented Mar 2, 2026 •

edited

Loading

Uh oh!

aikido-pr-checks bot Mar 2, 2026

Uh oh!

cubic-dev-ai bot left a comment

Uh oh!

codecov bot commented Mar 2, 2026

Uh oh!

cubic-dev-ai bot left a comment

Uh oh!

Uh oh!

shortstacked commented Mar 2, 2026

Uh oh!

Uh oh!

n8n-assistant bot commented Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

shortstacked commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Related Linear tickets, Github issues, and Community forum posts

Review / Merge checklist

Uh oh!

aikido-pr-checks bot Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 2, 2026

Codecov Report

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shortstacked commented Mar 2, 2026

Uh oh!

Uh oh!

n8n-assistant bot commented Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shortstacked commented Mar 2, 2026 •

edited

Loading