From 0e7ccf6f74288f291ab8ba7e345bca1615b3e36c Mon Sep 17 00:00:00 2001 From: Declan Carroll Date: Fri, 18 Sep 2026 08:33:32 +0000 Subject: [PATCH 01/67] test(engine): Add v2 infrastructure benchmarks (#38998) --- .../test-e2e-infrastructure-reusable.yml | 13 +-- AGENTS.md | 58 ++++++----- .../containers/services/engine-postgres.ts | 98 +++++++++++++++++++ .../testing/containers/services/engine.ts | 1 + .../testing/containers/services/postgres.ts | 15 +++ .../testing/containers/services/registry.ts | 3 + packages/testing/containers/services/types.ts | 1 + .../testing/containers/tests/engine.test.ts | 8 ++ packages/testing/playwright/AGENTS.md | 31 +++++- packages/testing/playwright/README.md | 19 +++- packages/testing/playwright/fixtures/base.ts | 6 +- .../testing/playwright/playwright-projects.ts | 4 + .../reporters/benchmark-summary-reporter.ts | 33 ++++++- .../scripts/distribution-images.mjs | 1 + .../tests/infrastructure/benchmarks/AGENTS.md | 37 +++++++ .../tests/infrastructure/benchmarks/README.md | 39 ++++++-- .../harness/kafka-backlog-harness.ts | 26 ++++- .../benchmarks/harness/load-harness.ts | 20 +++- .../benchmarks/harness/orchestration.ts | 33 ++++++- .../single-instance-webhook-harness.ts | 18 ++-- .../harness/webhook-throughput-harness.ts | 16 ++- ...tance-ceiling-engine-v2-lazy-cache.spec.ts | 48 +++++++++ ...nstance-ceiling-engine-v2-split-db.spec.ts | 42 ++++++++ .../single-instance-ceiling-engine-v2.spec.ts | 45 +++++++++ ...single-instance-ceiling-lazy-cache.spec.ts | 6 +- .../kafka/single-instance-ceiling.spec.ts | 6 +- ...ngle-instance-engine-v2-lazy-cache.spec.ts | 48 +++++++++ ...single-instance-engine-v2-split-db.spec.ts | 44 +++++++++ .../webhook-single-instance-engine-v2.spec.ts | 45 +++++++++ .../webhook-sync-latency-floor.spec.ts | 1 + .../expression-benchmark-profiles.ts | 56 ++++++++++- .../utils/benchmark/load-executors.ts | 6 +- .../utils/benchmark/sizing-matrix.ts | 11 ++- .../utils/benchmark/throughput-measure.ts | 16 ++- 34 files changed, 771 insertions(+), 83 deletions(-) create mode 100644 packages/testing/containers/services/engine-postgres.ts create mode 100644 packages/testing/playwright/tests/infrastructure/benchmarks/AGENTS.md create mode 100644 packages/testing/playwright/tests/infrastructure/benchmarks/kafka/single-instance-ceiling-engine-v2-lazy-cache.spec.ts create mode 100644 packages/testing/playwright/tests/infrastructure/benchmarks/kafka/single-instance-ceiling-engine-v2-split-db.spec.ts create mode 100644 packages/testing/playwright/tests/infrastructure/benchmarks/kafka/single-instance-ceiling-engine-v2.spec.ts create mode 100644 packages/testing/playwright/tests/infrastructure/benchmarks/webhook/webhook-single-instance-engine-v2-lazy-cache.spec.ts create mode 100644 packages/testing/playwright/tests/infrastructure/benchmarks/webhook/webhook-single-instance-engine-v2-split-db.spec.ts create mode 100644 packages/testing/playwright/tests/infrastructure/benchmarks/webhook/webhook-single-instance-engine-v2.spec.ts diff --git a/.github/workflows/test-e2e-infrastructure-reusable.yml b/.github/workflows/test-e2e-infrastructure-reusable.yml index 40236540c78..cc65362fd40 100644 --- a/.github/workflows/test-e2e-infrastructure-reusable.yml +++ b/.github/workflows/test-e2e-infrastructure-reusable.yml @@ -26,13 +26,14 @@ jobs: shards: '[{"shard":1,"images":""}]' - lane: webhook grep: '@bench:webhook' - # 2 shards — webhook lane grew from 6 to 9 spec files with the - # dedicated-proc topology (DEVP-200). Single shard pushed wall time - # past 40 min; two shards halves that. Bump to 3 if specs grow further. - shards: '[{"shard":1,"images":""},{"shard":2,"images":""}]' + # Three shards keep the runtime comparison profiles from extending + # this lane beyond the existing 25-minute target. + shards: '[{"shard":1,"images":""},{"shard":2,"images":""},{"shard":3,"images":""}]' - lane: kafka grep: '@bench:kafka' - shards: '[{"shard":1,"images":""},{"shard":2,"images":""}]' + # The v2 profiles add two isolated stacks. Keep lane wall time near + # the existing 22-minute target instead of serializing both profiles. + shards: '[{"shard":1,"images":""},{"shard":2,"images":""},{"shard":3,"images":""}]' uses: ./.github/workflows/test-e2e-reusable.yml with: test-mode: docker-artifact @@ -96,7 +97,7 @@ jobs: name: Notify Cats on failure runs-on: ubuntu-latest needs: [prepare-docker, benchmark, sizing-matrix] - if: ${{ always() && contains(needs.*.result, 'failure') }} + if: ${{ always() && github.event_name == 'schedule' && contains(needs.*.result, 'failure') }} permissions: contents: read steps: diff --git a/AGENTS.md b/AGENTS.md index e4eae7ed276..376c610588b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -173,7 +173,7 @@ The monorepo is organized into these key packages: - **Frontend:** Vue 3 + TypeScript + Vite + Pinia + Storybook UI Library - **Backend:** Node.js + TypeScript + Express + TypeORM -- **Testing:** Vitest (unit) + Playwright (E2E) +- **Testing:** Vitest (unit) + Playwright (UI, API, infrastructure, lifecycle, performance, and E2E orchestration) - **Database:** TypeORM with SQLite/PostgreSQL support - **Code Quality:** Biome (for formatting) + ESLint + lefthook git hooks @@ -327,33 +327,37 @@ extends one of those layers): - **data-testid must be a single value** (no spaces or multiple values) - Always use the `design-system` skill in reviews -### Testing Guidelines -- **Always work from within the package directory** when running tests -- **Mock all external dependencies** in unit tests -- **Prefer reusing hoisted shared `mock(...)` fixtures** when a typed mock is immutable and used across tests. This rule exists to avoid massive test slowdowns from repeatedly creating nested proxy mocks while preserving the type contract. Avoid replacing these with `as unknown as T` helpers for entities like `User`. -- **Confirm test cases with user** before writing unit tests -- **Typecheck is critical before committing** - always run `pnpm typecheck` -- **When modifying pinia stores**, check for unused computed properties -- **For Vitest packages that use `@n8n/di` decorators**, use `createVitestConfigWithDecorators` from `@n8n/vitest-config/node-decorators`. It enables SWC `decoratorMetadata` (esbuild doesn't emit it) and externalizes workspace packages that register services (`@n8n/di`, `@n8n/config`, `@n8n/constants`, `n8n-workflow`) so a single DI `Container` instance is shared across the runtime. Loading them through Vitest's pipeline alongside their CJS dist produces two `Container`s and `Container.get(...)` returns `undefined`. +### Testing and Local Development -What we use for testing and writing tests: -- For testing nodes and other backend components, we use Vitest for unit tests. Examples can be found in `packages/nodes-base/nodes/**/*test*`. -- We use `nock` for server mocking -- For frontend we use `vitest` -- For E2E tests we use Playwright. Run with `pnpm --filter=n8n-playwright test:local`. - See `packages/testing/playwright/README.md` for details. -- **To iterate on a feature without docker rebuilds**, boot service containers - and run the dev servers locally — `pnpm --filter n8n-containers services --services postgres,redis,mailpit,proxy` - then `pnpm dev:be` (backend on 5678). For frontend hot reload, also run - `pnpm dev:fe:editor` (8080). The root `pnpm dev` does not exist: it prints a - notice and exits with code 0, thus `pnpm dev && …` looks successful but no - server runs. See - [Develop against running containers](packages/testing/playwright/README.md#develop-against-running-containers-avoid-docker-rebuilds). -- **In a codespace agent session**, use `pnpm dev:up`. It installs the missing - dependencies, starts the backend, waits for health, shares the port with the - org, and prints the URL. See - [.devcontainer/codespaces/README.md](.devcontainer/codespaces/README.md). -- **For Playwright test maintenance/cleanup**, see `packages/testing/playwright/AGENTS.md` (includes janitor tool for static analysis, dead code removal, architecture enforcement, and TCR workflows). +Choose the smallest runner that owns the behavior: + +| Need | Use | +|------|-----| +| Unit or component behavior | Vitest from the owning package | +| UI, API, lifecycle, topology, or performance orchestration | Playwright; read `packages/testing/playwright/AGENTS.md` | +| Add a test service, capability, or managed stack | Read `packages/testing/containers/README.md` | + +Testing rules: + +- Run tests and `pnpm typecheck` from the owning package. +- Confirm unit test cases with the user before you write them. +- Mock external dependencies. Use `nock` for HTTP services. +- Reuse immutable hoisted `mock(...)` fixtures. Do not replace typed entity mocks with `as unknown as T`. +- Use `createVitestConfigWithDecorators` for Vitest packages that use `@n8n/di` decorators. +- Check for unused computed properties after you change a Pinia store. + +Choose a local development path: + +| Goal | Command | +|------|---------| +| Run product E2E against a local instance | `pnpm --filter=n8n-playwright test:local` | +| Run backend with PostgreSQL, Redis, email, and proxy services | `pnpm --filter n8n-containers services --services postgres,redis,mailpit,proxy`, then `pnpm dev:be` | +| Add editor hot reload | `pnpm dev:fe:editor` | +| Start a Codespace backend and share its port | `pnpm dev:up` | + +The root `pnpm dev` command does not start a server. See the +[Playwright guide](packages/testing/playwright/README.md) and the +[Codespaces guide](.devcontainer/codespaces/README.md) for details. ### Common Development Tasks diff --git a/packages/testing/containers/services/engine-postgres.ts b/packages/testing/containers/services/engine-postgres.ts new file mode 100644 index 00000000000..1816c417a1a --- /dev/null +++ b/packages/testing/containers/services/engine-postgres.ts @@ -0,0 +1,98 @@ +import { PostgreSqlContainer } from '@testcontainers/postgresql'; +import type { StartedNetwork } from 'testcontainers'; + +import { createSilentLogConsumer } from '../helpers/utils'; +import { TEST_CONTAINER_IMAGES } from '../test-containers'; +import { ENGINE_DATABASE } from './engine'; +import { PostgresHelper, type PostgresResult } from './postgres'; +import type { HelperContext, Service, StartContext } from './types'; + +const HOSTNAME = 'engine-postgres'; +const USERNAME = 'n8n_user'; +const PASSWORD = 'test_password'; + +export const enginePostgres: Service = { + description: 'Engine PostgreSQL database', + + async start( + network: StartedNetwork, + projectName: string, + _options?: unknown, + ctx?: StartContext, + ) { + const { consumer, throwWithLogs } = createSilentLogConsumer(); + try { + const container = await new PostgreSqlContainer(TEST_CONTAINER_IMAGES.postgres) + .withNetwork(network) + .withNetworkAliases(HOSTNAME) + .withDatabase(ENGINE_DATABASE) + .withUsername(USERNAME) + .withPassword(PASSWORD) + .withStartupTimeout(60000) + .withLabels({ + 'com.docker.compose.project': projectName, + 'com.docker.compose.service': HOSTNAME, + }) + .withName(`${projectName}-${HOSTNAME}`) + .withTmpFs({ '/var/lib/postgresql': 'rw' }) + .withCommand([ + 'postgres', + '-c', + 'fsync=off', + '-c', + 'synchronous_commit=off', + '-c', + 'full_page_writes=off', + '-c', + 'max_connections=200', + '-c', + 'shared_preload_libraries=pg_stat_statements', + '-c', + 'pg_stat_statements.track=all', + '-c', + 'pg_stat_statements.track_planning=on', + '-c', + 'track_io_timing=on', + ]) + .withReuse() + .withLogConsumer(consumer) + .start(); + ctx?.registerContainer?.(container); + await container.exec([ + 'psql', + '-U', + USERNAME, + '-d', + ENGINE_DATABASE, + '-c', + 'CREATE EXTENSION IF NOT EXISTS pg_stat_statements;', + ]); + return { + container, + meta: { database: ENGINE_DATABASE, username: USERNAME, password: PASSWORD }, + }; + } catch (error: unknown) { + return throwWithLogs(error); + } + }, + + env(result: PostgresResult, external?: boolean) { + const host = external ? result.container.getHost() : HOSTNAME; + const port = external ? result.container.getMappedPort(5432) : 5432; + return { + N8N_ENGINE_DATABASE_URL: `postgres://${USERNAME}:${PASSWORD}@${host}:${port}/${ENGINE_DATABASE}`, + }; + }, +}; + +export function createEnginePostgresHelper(ctx: HelperContext): PostgresHelper | undefined { + const result = ctx.serviceResults.enginePostgres as PostgresResult | undefined; + if (!result) return undefined; + return new PostgresHelper(result.container, result.meta); +} + +declare module './types' { + interface ServiceHelpers { + enginePostgres?: PostgresHelper; + } +} diff --git a/packages/testing/containers/services/engine.ts b/packages/testing/containers/services/engine.ts index d842b22f360..7456ba46cee 100644 --- a/packages/testing/containers/services/engine.ts +++ b/packages/testing/containers/services/engine.ts @@ -70,6 +70,7 @@ export function applyEngineEnv( const modules = (env.N8N_ENABLED_MODULES ?? '').split(',').filter(Boolean); if (!modules.includes(ENGINE_MODULE)) modules.push(ENGINE_MODULE); env.N8N_ENABLED_MODULES = modules.join(','); + if (env.N8N_ENGINE_DATABASE_URL) return; const user = encodeURIComponent(env.DB_POSTGRESDB_USER); const password = encodeURIComponent(env.DB_POSTGRESDB_PASSWORD); diff --git a/packages/testing/containers/services/postgres.ts b/packages/testing/containers/services/postgres.ts index e5651aa735d..d2e81ff0c19 100644 --- a/packages/testing/containers/services/postgres.ts +++ b/packages/testing/containers/services/postgres.ts @@ -211,6 +211,21 @@ export class PostgresHelper { await this.exec('SELECT pg_stat_statements_reset();'); } + async countCompletedEngineV2Executions(workflowId: string): Promise { + if (!/^[A-Za-z0-9_-]+$/.test(workflowId)) { + throw new Error(`Unexpected workflow ID: ${workflowId}`); + } + const { output } = await this.runIn( + ENGINE_DATABASE, + ` + SELECT COUNT(*)::bigint + FROM workflow_execution + WHERE status = 'completed' AND workflow_id = '${workflowId}'; + `, + ); + return parseInt(output.trim(), 10); + } + /** Top N statements ordered by call count descending. */ async topStatements(limit = 15): Promise< Array<{ diff --git a/packages/testing/containers/services/registry.ts b/packages/testing/containers/services/registry.ts index 32fe2e4c262..6fde1e43d00 100644 --- a/packages/testing/containers/services/registry.ts +++ b/packages/testing/containers/services/registry.ts @@ -1,5 +1,6 @@ import { cadvisor } from './cadvisor'; import { cloudflared } from './cloudflared'; +import { createEnginePostgresHelper, enginePostgres } from './engine-postgres'; import { gitea, createGiteaHelper } from './gitea'; import { kafka, createKafkaHelper } from './kafka'; import { kent, createKentHelper } from './kent'; @@ -25,6 +26,7 @@ import { victoriaMetrics } from './victoria-metrics'; /** Service registry - must include all ServiceName entries */ export const services: Record> = { postgres, + enginePostgres, redis, mailpit, gitea, @@ -49,6 +51,7 @@ export const services: Record> = { export const helperFactories: Partial = { postgres: createPostgresHelper, + enginePostgres: createEnginePostgresHelper, mailpit: createMailpitHelper, gitea: createGiteaHelper, keycloak: createKeycloakHelper, diff --git a/packages/testing/containers/services/types.ts b/packages/testing/containers/services/types.ts index 43491da19b7..35fae2b1f69 100644 --- a/packages/testing/containers/services/types.ts +++ b/packages/testing/containers/services/types.ts @@ -7,6 +7,7 @@ export const EXTERNAL_HOST = 'host.docker.internal'; export const SERVICE_NAMES = [ 'postgres', + 'enginePostgres', 'redis', 'mailpit', 'gitea', diff --git a/packages/testing/containers/tests/engine.test.ts b/packages/testing/containers/tests/engine.test.ts index 08ea338c3c9..2e47c2fd98c 100644 --- a/packages/testing/containers/tests/engine.test.ts +++ b/packages/testing/containers/tests/engine.test.ts @@ -46,6 +46,14 @@ describe('applyEngineEnv', () => { ); }); + test('keeps an explicit data plane database URL', () => { + const env = { ...postgresEnv, N8N_ENGINE_DATABASE_URL: 'postgres://engine-postgres/db' }; + + applyEngineEnv(env, { engine: 'in-process', isQueueMode: false }); + + expect(env.N8N_ENGINE_DATABASE_URL).toBe('postgres://engine-postgres/db'); + }); + test('rejects a stack without Postgres', () => { const env: Record = { DB_TYPE: 'sqlite' }; diff --git a/packages/testing/playwright/AGENTS.md b/packages/testing/playwright/AGENTS.md index d7236a82160..8baa1e84315 100644 --- a/packages/testing/playwright/AGENTS.md +++ b/packages/testing/playwright/AGENTS.md @@ -1,5 +1,27 @@ # AGENTS.md +## Purpose + +Playwright is n8n's general-purpose test orchestrator. Do not assume that every +Playwright test drives the editor UI. This package also owns API tests, container +topologies, process lifecycle tests, infrastructure validation, performance +benchmarks, evaluation suites, and browser-backed harness contracts. + +Choose the suite by the behavior under test: + +| Behavior | Location | Runner | +|----------|----------|--------| +| Product UI or API journey | `tests/e2e/` | `test:local` or a container project | +| Database, queue, multi-main, encryption, or process lifecycle | `tests/infrastructure/` | `test:infrastructure` or the named project | +| Infrastructure throughput and resource use | `tests/infrastructure/benchmarks/` | `test:benchmark` | +| Browser and canvas performance | `tests/performance/` | `test:performance` | +| Fixture or harness contract | `tests/framework/` | `test:unit` or `test:harness` | +| Evaluation scenario | Existing evaluation directory | Its named evaluation project | + +Use Playwright when the test needs its worker lifecycle, fixtures, retries, +artifacts, project matrix, browser context, or managed container stack. Use +Vitest for browser-free unit and integration tests that need none of these. + ## Commands ```bash @@ -10,6 +32,9 @@ pnpm --filter=n8n-playwright test:local tests/e2e/credentials/crud.spec.ts # Run with container capabilities (requires pnpm build:docker first) pnpm --filter=n8n-playwright test:container:sqlite tests/e2e/auth/password-reset.spec.ts +# Run one infrastructure benchmark +pnpm --filter=n8n-playwright test:benchmark tests/infrastructure/benchmarks/kafka/single-instance-ceiling.spec.ts + # Lint and typecheck pnpm --filter=n8n-playwright lint pnpm --filter=n8n-playwright typecheck @@ -177,9 +202,11 @@ pnpm janitor --file=tests/my-new-test.spec.ts --verbose See `packages/testing/janitor/README.md` for full documentation. -## Entry Points +## UI Entry Points -All tests should start with `n8n.start.*` methods. See `composables/TestEntryComposer.ts`. +UI journey tests should start with `n8n.start.*` methods. API, infrastructure, +benchmark, lifecycle, and framework tests use their own fixtures and harnesses. +See `composables/TestEntryComposer.ts` for UI entry points. | Method | Use Case | |--------|----------| diff --git a/packages/testing/playwright/README.md b/packages/testing/playwright/README.md index 908ce4c0b56..0b5151b1b09 100644 --- a/packages/testing/playwright/README.md +++ b/packages/testing/playwright/README.md @@ -1,4 +1,21 @@ -# Playwright E2E Test Guide +# Playwright Test Orchestration Guide + +Playwright is not only the browser E2E runner. This package uses Playwright's +worker lifecycle and project model to orchestrate browsers, APIs, n8n processes, +service containers, deployment topologies, diagnostics, and benchmark artifacts. + +## Choose a suite + +| Goal | Start here | +|------|------------| +| Test a product UI or API journey | `tests/e2e/` | +| Test PostgreSQL, queue mode, multi-main, encryption, or process lifecycle | `tests/infrastructure/` | +| Measure infrastructure throughput or resource use | [`tests/infrastructure/benchmarks/README.md`](tests/infrastructure/benchmarks/README.md) | +| Measure browser or canvas performance | `tests/performance/README.md` | +| Test fixtures, startup, telemetry, or harness behavior | `tests/framework/` | + +Read `AGENTS.md` before you add a test. It defines suite boundaries, fixtures, +container capabilities, and required verification. ## Development setup ```bash diff --git a/packages/testing/playwright/fixtures/base.ts b/packages/testing/playwright/fixtures/base.ts index b56f08ce6c8..b285b7eb425 100644 --- a/packages/testing/playwright/fixtures/base.ts +++ b/packages/testing/playwright/fixtures/base.ts @@ -235,7 +235,11 @@ export const test = base.extend< await apiContext.dispose(); // The reset endpoint only reaches the control plane database. - if (n8nStackConfig.engine) await n8nContainer.services.postgres.truncateEngineDatabase(); + if (n8nStackConfig.engine) { + await ( + n8nContainer.services.enginePostgres ?? n8nContainer.services.postgres + ).truncateEngineDatabase(); + } } await use(undefined); }, diff --git a/packages/testing/playwright/playwright-projects.ts b/packages/testing/playwright/playwright-projects.ts index 0decc5585e4..135de5ad49e 100644 --- a/packages/testing/playwright/playwright-projects.ts +++ b/packages/testing/playwright/playwright-projects.ts @@ -91,6 +91,8 @@ export interface BenchOptions { * collecting flamegraph data, not for clean ceiling numbers. */ tracing?: boolean; + /** Runs engine v2 on the shared Postgres server or a separate server. */ + engine?: 'shared' | 'split'; /** Additional env vars to merge over the base. */ env?: Record; } @@ -114,6 +116,7 @@ export function benchConfig(isolation: string, opts: BenchOptions = {}): N8NConf const services = [...(BENCHMARK_CONFIG.services ?? [])]; if (opts.kafka) services.push('kafka'); if (opts.tracing) services.push('tracing'); + if (opts.engine === 'split') services.push('enginePostgres'); const env: Record = { ...BENCHMARK_CONFIG.env, @@ -134,6 +137,7 @@ export function benchConfig(isolation: string, opts: BenchOptions = {}): N8NConf ...(opts.mains !== undefined && { mains: opts.mains }), ...(opts.workers !== undefined && { workers: opts.workers }), ...(opts.webhooks !== undefined && { webhooks: opts.webhooks }), + ...(opts.engine !== undefined && { engine: 'in-process' as const }), env, }; } diff --git a/packages/testing/playwright/reporters/benchmark-summary-reporter.ts b/packages/testing/playwright/reporters/benchmark-summary-reporter.ts index c73a53353ff..dc686e40051 100644 --- a/packages/testing/playwright/reporters/benchmark-summary-reporter.ts +++ b/packages/testing/playwright/reporters/benchmark-summary-reporter.ts @@ -3,6 +3,7 @@ import { appendFileSync } from 'fs'; interface BenchmarkRow { question: string; + profile: string; variant: string; verdict: string; scenario: string; @@ -16,6 +17,11 @@ interface Column { } const COLUMNS: Column[] = [ + { + header: 'complete', + suffixes: ['completion-pct'], + format: (v) => `${v.toFixed(1)}%`, + }, { header: 'exec/s', suffixes: ['throughput', 'exec-per-sec'], @@ -56,6 +62,21 @@ const COLUMNS: Column[] = [ suffixes: ['event-loop-lag'], format: (v) => `${(v * 1000).toFixed(0)}ms`, }, + { + header: 'CPU', + suffixes: ['main-cpu-avg'], + format: (v) => `${v.toFixed(0)}%`, + }, + { + header: 'RAM', + suffixes: ['main-memory-mb'], + format: (v) => `${v.toFixed(0)}MB`, + }, + { + header: 'PG CPU', + suffixes: ['pg-cpu-avg'], + format: (v) => `${v.toFixed(0)}%`, + }, { header: 'pg tx/s', suffixes: ['pg-tx-rate'], @@ -106,6 +127,7 @@ interface QuestionShape { columns: Column[]; showVariant: boolean; showVerdict: boolean; + showProfile: boolean; } function shapeFor(rows: BenchmarkRow[]): QuestionShape { @@ -113,6 +135,7 @@ function shapeFor(rows: BenchmarkRow[]): QuestionShape { columns: COLUMNS.filter((col) => rows.some((r) => col.suffixes.some((s) => r.metrics.has(s)))), showVariant: rows.some((r) => r.variant.length > 0), showVerdict: rows.some((r) => r.verdict.length > 0), + showProfile: rows.some((r) => r.profile.length > 0), }; } @@ -156,6 +179,7 @@ class BenchmarkSummaryReporter implements Reporter { this.rows.push({ question, + profile: String(dimensions.runtime_profile ?? ''), variant: String(dimensions.variant ?? ''), verdict: String(dimensions.verdict ?? ''), scenario, @@ -204,12 +228,14 @@ class BenchmarkSummaryReporter implements Reporter { const scenario = rows[0]?.scenario ?? ''; const headerCells: string[] = []; + if (shape.showProfile) headerCells.push('Profile'); if (shape.showVariant) headerCells.push('Variant'); if (shape.showVerdict) headerCells.push('Verdict'); for (const col of shape.columns) headerCells.push(col.header); const rowCells: string[][] = rows.map((row) => { const cells: string[] = []; + if (shape.showProfile) cells.push(row.profile || '—'); if (shape.showVariant) cells.push(row.variant || '—'); if (shape.showVerdict) cells.push(row.verdict || '—'); for (const col of shape.columns) cells.push(formatCell(row, col)); @@ -225,8 +251,9 @@ class BenchmarkSummaryReporter implements Reporter { leftAlign ? padRight(s, w) : pad(s, w); // Variant + Verdict are left-aligned text; metric columns are right-aligned numbers. - const isText = (i: number) => - (shape.showVariant && i === 0) || (shape.showVerdict && i === (shape.showVariant ? 1 : 0)); + const textColumns = + Number(shape.showProfile) + Number(shape.showVariant) + Number(shape.showVerdict); + const isText = (i: number) => i < textColumns; console.log(''); console.log(`▎ ${question}`); @@ -256,6 +283,7 @@ class BenchmarkSummaryReporter implements Reporter { lines.push(''); const headers: string[] = []; + if (shape.showProfile) headers.push('Profile'); if (shape.showVariant) headers.push('Variant'); if (shape.showVerdict) headers.push('Verdict'); for (const col of shape.columns) headers.push(col.header); @@ -265,6 +293,7 @@ class BenchmarkSummaryReporter implements Reporter { for (const row of rows) { const cells: string[] = []; + if (shape.showProfile) cells.push(row.profile || '—'); if (shape.showVariant) cells.push(row.variant || '—'); if (shape.showVerdict) cells.push(row.verdict || '—'); for (const col of shape.columns) cells.push(formatCell(row, col)); diff --git a/packages/testing/playwright/scripts/distribution-images.mjs b/packages/testing/playwright/scripts/distribution-images.mjs index 36a1d24659b..0ff89e18ed2 100644 --- a/packages/testing/playwright/scripts/distribution-images.mjs +++ b/packages/testing/playwright/scripts/distribution-images.mjs @@ -24,6 +24,7 @@ const SERVICE_IMAGES = { mailpit: ['mailpit'], mysql: ['mysql'], ngrok: ['ngrok'], + enginePostgres: ['postgres'], postgres: ['postgres'], postgresExporter: ['postgresExporter'], proxy: ['mockserver'], diff --git a/packages/testing/playwright/tests/infrastructure/benchmarks/AGENTS.md b/packages/testing/playwright/tests/infrastructure/benchmarks/AGENTS.md new file mode 100644 index 00000000000..56026ddcebe --- /dev/null +++ b/packages/testing/playwright/tests/infrastructure/benchmarks/AGENTS.md @@ -0,0 +1,37 @@ +# Infrastructure Benchmark Instructions + +Read `README.md` before you change this suite. + +## Purpose + +These Playwright specs use the worker lifecycle as a benchmark orchestrator. +They create isolated container stacks, generate load, sample completions and +resources, attach structured reports, and render the GitHub job summary. Most +specs do not use a browser. + +## Rules + +- Give each spec one scaling question. +- Configure topology with `benchConfig()` in the spec. +- Reuse the Kafka or webhook harness. Do not copy orchestration logic into a spec. +- Add an owner and a unique `question` annotation. +- Set `dimensions.variant` when one summary contains multiple rows. +- Keep runtime comparison profiles out of the deployment sizing matrix. +- Treat v2 saturation profiles as measurements. Require progress, not full drain. +- Keep exact completion checks for v1 delivery baselines. +- Record a new comparison axis as a metric dimension. +- Add a shard when a lane would exceed its existing wall-time target. +- Send Slack failure notifications only for scheduled runs. + +## Verification + +```bash +pnpm lint +pnpm typecheck +pnpm test:unit +pnpm janitor +pnpm test:benchmark +``` + +Run commands from `packages/testing/playwright`. Build the n8n image first when +the benchmark must include product-code changes. diff --git a/packages/testing/playwright/tests/infrastructure/benchmarks/README.md b/packages/testing/playwright/tests/infrastructure/benchmarks/README.md index f7dc1a0d203..0ea086c5444 100644 --- a/packages/testing/playwright/tests/infrastructure/benchmarks/README.md +++ b/packages/testing/playwright/tests/infrastructure/benchmarks/README.md @@ -16,9 +16,16 @@ The architectural ceiling. No queue tax, no worker dispatch. What's the absolute |---------|------|----------| | kafka | `single-instance-ceiling.spec.ts` | How much can we process on a single instance? | | kafka | `single-instance-ceiling-lazy-cache.spec.ts` | How much can lazy expression isolates process? | +| kafka | `single-instance-ceiling-engine-v2.spec.ts` | How much can engine v2 process with eager expression isolates? | +| kafka | `single-instance-ceiling-engine-v2-lazy-cache.spec.ts` | How much can engine v2 process with lazy expression isolates? | +| kafka | `single-instance-ceiling-engine-v2-split-db.spec.ts` | Does a separate engine database server change v2 throughput? | | kafka | `steady-rate-breaking-point.spec.ts` | At what input rate does the system fall behind? | | webhook | `webhook-single-instance.spec.ts` | What is the single-instance webhook ingestion ceiling? | | webhook | `webhook-single-instance-lazy-cache.spec.ts` | What is the lazy expression-isolate webhook ceiling? | +| webhook | `webhook-single-instance-engine-v2.spec.ts` | What is the engine v2 webhook ceiling with eager isolates? | +| webhook | `webhook-single-instance-engine-v2-lazy-cache.spec.ts` | What is the engine v2 webhook ceiling with lazy isolates? | +| webhook | `webhook-single-instance-engine-v2-split-db.spec.ts` | Does a separate engine database server change v2 webhook throughput? | +| webhook | `webhook-sync-latency-floor.spec.ts` | How does connection count affect synchronous webhook latency? | ### Actual — `1m + 1wp + 1w queue mode` @@ -64,26 +71,33 @@ Webhook-trigger specs in **Actual** and **Scaling** use the production-canonical All specs share a single env profile aligned with internal n8n production defaults — connection-pool, lock-duration, and Bull/Redis tuning from real deployments. See `BENCHMARK_CONFIG` in `playwright-projects.ts`. -### Expression engine profiles +### Runtime comparison profiles -The direct Kafka and webhook baselines run with two explicit VM expression-engine profiles: +The direct Kafka and webhook baselines run with two explicit VM expression-engine profiles and an execution engine v2 comparison: -| Profile | Lazy acquisition | Compile cache | Purpose | -|---------|------------------|---------------|---------| -| `vm-eager` | Off | Off | Tracks the current default execution path. | -| `vm-lazy-cache` | On | On | Tracks the optimized no-expression execution path. | +| Profile | Execution engine | Expression engine | Lazy acquisition | Compile cache | Purpose | +|---------|------------------|-------------------|------------------|---------------|---------| +| `vm-eager` | v1 | VM | Off | Off | Tracks the current default execution path. | +| `vm-lazy-cache` | v1 | VM | On | On | Tracks the optimized no-expression execution path. | +| `engine-v2` | v2 | VM | Off | Off | Tracks the new workflow execution engine with the same expression settings as `vm-eager`. | +| `engine-v2-vm-lazy-cache` | v2 | VM | On | On | Measures the combined execution engine v2 and optimized expression-isolate path. | +| `engine-v2-split-db-vm-lazy-cache` | v2 | VM | On | On | Runs the data plane on a separate PostgreSQL server. | -Each metric records `expression_engine`, `expression_lazy_acquire`, `expression_compile_cache`, and `expression_profile` dimensions. The lazy comparison runs remain in benchmark telemetry and run-report artifacts, but do not feed the deployment sizing matrix while eager VM is the default runtime. +Each metric records `execution_engine`, `expression_engine`, `expression_lazy_acquire`, `expression_compile_cache`, and `expression_profile` dimensions. The lazy and engine v2 comparison runs remain in benchmark telemetry and run-report artifacts, but do not feed the deployment sizing matrix while execution engine v1 with eager VM is the default runtime. These NoOp workflows do not evaluate expressions. Lazy mode therefore avoids acquiring an isolate, so these comparisons primarily measure lazy acquisition. Use the expression-engine microbenchmarks to measure compile-cache behavior directly. +The engine v2 webhook comparison uses the same 120-second ingestion load as the v1 profiles. It does not wait for the accepted backlog to drain. Compare its tail execution rate and backlog growth instead of its completion ratio. + +The engine v2 Kafka comparisons report partial completion without enforcing a completion ratio. They require only observable progress because these runs measure throughput under saturation. The v1 baselines still require exact completion. + ## Running ```bash # Build n8n image first (skip if you only changed test code). pnpm build:docker -# Full suite — all 15 specs sequentially (each spawns its own container). +# Full suite — all 21 specs sequentially (each spawns its own container). pnpm --filter=n8n-playwright test:benchmark # One spec. @@ -117,14 +131,20 @@ Every run prints a per-test `[DIAG]` block and emits a Benchmark Summary table a | Column | Meaning | |--------|---------| | `exec/s` | Workflow executions per second across the active window | +| `complete` | Percentage of the published finite load observed as completed | | `tail/s` | Throughput across the final 60s of the run — closest to the architectural ceiling | | `actions/s` | `exec/s × nodeCount` — total node executions per second | | `p50/p99` | Per-execution duration percentiles (when execution data is saved) | | `req/s` | HTTP requests per second (webhook specs only) | | `ev lag` | Node.js event loop lag (sum across mains/workers) | +| `CPU` | Average CPU use across main containers | +| `RAM` | Average memory use across main containers | +| `PG CPU` | Average control-plane PostgreSQL container CPU use | | `pg tx/s` | Postgres `xact_commit` rate from postgres-exporter | | `queue` | Bull jobs waiting (queue specs only) | +Runtime comparison rows include a `Profile` column such as `v1 eager`, `v1 lazy/cache`, or `v2 lazy/cache, split DB`. + Tail and staged rates require at least three distinct counter samples and 80% coverage of the requested window. Duplicate polls between VictoriaMetrics scrapes do not count as samples. A short or incomplete window omits the tail metric instead of reporting a whole-run fallback or zero. Kafka stage rates use the publisher's actual boundaries. For deeper PG analysis, every spec also logs a top-N `pg_stat_statements` breakdown ranked by total ms/s of work (calls/s × avg ms), plus a `[PG SATURATION]` block (total query CPU including planner overhead and the long tail, buffer hit ratio, bgwriter / WAL pressure, `pg_stat_io` per-backend-type IO) and a `[CONTAINERS]` block (per-container CPU/memory/IO from cAdvisor or `docker stats` sampler). Each run also attaches a `run-report.json` artifact with the full structured report — feedable directly to an LLM for bottleneck analysis. @@ -158,6 +178,8 @@ Adding a new trigger type requires one driver + one or more spec files. The harn ## Adding a spec +Agents must also follow `AGENTS.md` in this directory. + 1. Pick a question that isn't already answered by an existing spec. 2. Decide which tier it belongs to: **Peak** (no workers), **Actual** (1m+1w), or **Scaling** (2m+2w). 3. Create `kafka/.spec.ts` or `webhook/.spec.ts`. @@ -168,3 +190,4 @@ Adding a new trigger type requires one driver + one or more spec files. The harn - Scaling: `benchConfig('', { mains: 2, workers: 2 })` (kafka adds `kafka: true`) 5. Wire the trigger driver (`kafkaDriver` or `setupWebhook`) and a harness (`runLoadTest` or `runWebhookThroughputTest`). 6. Annotate with `{ type: 'question', description: '' }` so the question is searchable in test metadata. +7. Set `variant` when the question emits multiple rows. The summary reporter does not infer row labels from other dimensions. diff --git a/packages/testing/playwright/tests/infrastructure/benchmarks/harness/kafka-backlog-harness.ts b/packages/testing/playwright/tests/infrastructure/benchmarks/harness/kafka-backlog-harness.ts index 2eee992b875..634b27648ab 100644 --- a/packages/testing/playwright/tests/infrastructure/benchmarks/harness/kafka-backlog-harness.ts +++ b/packages/testing/playwright/tests/infrastructure/benchmarks/harness/kafka-backlog-harness.ts @@ -7,9 +7,11 @@ import { kafkaDriver, resolveMetricQuery, waitForThroughput, + WORKFLOW_SUCCESS_QUERY, } from '../../../../utils/benchmark'; import type { BenchmarkDimensions, + CompletionCounterReader, ExecutionMetrics, LoadProfile, TriggerSetupContext, @@ -30,7 +32,9 @@ interface KafkaLoadOptions { variant?: string; minTailRateEfficiency?: number; requireKeptUpStage?: boolean; + requireTailMeasurement?: boolean; dimensions?: BenchmarkDimensions; + counterReader?: CompletionCounterReader; } export async function runKafkaLoadTest(options: KafkaLoadOptions): Promise { @@ -53,11 +57,13 @@ export async function runKafkaLoadTest(options: KafkaLoadOptions): Promise { + counterReader: options.counterReader, + warmUp: async ({ counterReader }) => { const metrics = options.services.observability.metrics; const metricQuery = resolveMetricQuery(options.testInfo); - const baselineCounter = await getBaselineCounter(metrics, metricQuery); + const baselineCounter = await getBaselineCounter(metrics, metricQuery, counterReader); const published = await handle.preload(WARMUP_MESSAGE_COUNT); const result = await waitForThroughput(metrics, { expectedCount: published.totalPublished, @@ -65,6 +71,7 @@ export async function runKafkaLoadTest(options: KafkaLoadOptions): Promise & { messageCount: number }, + options: Omit & { + messageCount: number; + }, ): Promise { await runKafkaLoadTest({ ...options, - scenario: { nodeCount: 1, payloadSize: '1KB', nodeOutputSize: 'noop', partitions: 3 }, + counterReader: + options.dimensions?.execution_engine === 'v1' + ? async () => await options.api.metrics.getCounter(WORKFLOW_SUCCESS_QUERY) + : undefined, + scenario: { + nodeCount: 1, + payloadSize: '1KB', + nodeOutputSize: 'noop', + partitions: 3, + }, load: { type: 'preloaded', count: options.messageCount }, }); } diff --git a/packages/testing/playwright/tests/infrastructure/benchmarks/harness/load-harness.ts b/packages/testing/playwright/tests/infrastructure/benchmarks/harness/load-harness.ts index 84c8c22a111..117a6a986d6 100644 --- a/packages/testing/playwright/tests/infrastructure/benchmarks/harness/load-harness.ts +++ b/packages/testing/playwright/tests/infrastructure/benchmarks/harness/load-harness.ts @@ -50,6 +50,8 @@ export interface LoadTestOptions { trigger: TriggerType; /** PromQL metric to track workflow completions. Defaults to resolveMetricQuery(testInfo). */ metricQuery?: string; + /** Direct completion reader for single-instance comparisons. */ + counterReader?: SetupContext['counterReader']; /** Additional dimensions attached to every metric and the run report. */ dimensions?: BenchmarkDimensions; /** When provided, the result log includes a resource breakdown (mode/main/workers/total). */ @@ -69,6 +71,8 @@ export interface LoadTestOptions { minTailRateEfficiency?: number; /** Fail unless one stage keeps up with at least 95% of its requested rate. */ requireKeptUpStage?: boolean; + /** Fail unless the sampler produces a covered tail-rate measurement. */ + requireTailMeasurement?: boolean; } /** @@ -97,6 +101,7 @@ export async function runLoadTest(options: LoadTestOptions): Promise Promise; + warmUp?: (setup: { + workflowId: string; + webhookPath?: string; + counterReader?: CompletionCounterReader; + }) => Promise; } export interface SetupResult { @@ -72,6 +79,7 @@ export interface SetupResult { webhookPath?: string; baselineCounter: number; metricQuery: string; + counterReader?: CompletionCounterReader; /** Wall-clock when the workflow finished activating — useful as a fallback timer. */ activationStart: number; /** Cumulative `pg_stat_wal` counters captured post-warm-up. Diffed at end-of-run. */ @@ -102,6 +110,13 @@ export async function setupBenchmarkRun(ctx: SetupContext): Promise makeUnique: ctx.createOptions?.makeUnique ?? true, webhookPrefix: ctx.createOptions?.webhookPrefix, }); + const counterReader = + ctx.api.options.workflowSettings?.engineType === 'v2' + ? async () => + await ( + ctx.services.enginePostgres ?? ctx.services.postgres + ).countCompletedEngineV2Executions(workflowId) + : ctx.counterReader; // VictoriaMetrics needs at least one scrape before queries return data. await obs.metrics.waitForMetric('n8n_version_info', { @@ -116,10 +131,10 @@ export async function setupBenchmarkRun(ctx: SetupContext): Promise await ctx.handle.waitForReady({ timeoutMs: 30_000 }); } if (ctx.warmUp) { - await ctx.warmUp({ workflowId, webhookPath }); + await ctx.warmUp({ workflowId, webhookPath, counterReader }); } - const baselineCounter = await getBaselineCounter(obs.metrics, metricQuery); + const baselineCounter = await getBaselineCounter(obs.metrics, metricQuery, counterReader); // Best-effort: the postgres helper may not be wired or pg_stat_statements // may not be available on a custom postgres image. @@ -151,6 +166,7 @@ export async function setupBenchmarkRun(ctx: SetupContext): Promise webhookPath, baselineCounter, metricQuery, + counterReader, activationStart, walBaseline, dockerStatsSampler, @@ -631,6 +647,17 @@ export async function attachReportMetrics( if (mainCpu !== undefined) { await attachMetric(testInfo, 'main-cpu-avg', mainCpu, '%', dimensions); } + const mainContainers = report.containers.filter((container) => + container.name.startsWith('n8n-main'), + ); + if (mainContainers.length > 0) { + const mainMemoryMb = + mainContainers.reduce((sum, container) => sum + (container.memBytes ?? 0), 0) / + mainContainers.length / + 1024 / + 1024; + await attachMetric(testInfo, 'main-memory-mb', mainMemoryMb, 'MB', dimensions); + } const workerCpu = avgCpuFor('n8n-worker'); if (workerCpu !== undefined) { await attachMetric(testInfo, 'worker-cpu-avg', workerCpu, '%', dimensions); diff --git a/packages/testing/playwright/tests/infrastructure/benchmarks/harness/single-instance-webhook-harness.ts b/packages/testing/playwright/tests/infrastructure/benchmarks/harness/single-instance-webhook-harness.ts index d93508a0d99..0590f0265aa 100644 --- a/packages/testing/playwright/tests/infrastructure/benchmarks/harness/single-instance-webhook-harness.ts +++ b/packages/testing/playwright/tests/infrastructure/benchmarks/harness/single-instance-webhook-harness.ts @@ -3,6 +3,7 @@ import type { ServiceHelpers } from 'n8n-containers/services/types'; import type { ApiHelpers } from '../../../../services/api-helper'; import type { BenchmarkDimensions } from '../../../../utils/benchmark'; +import { WORKFLOW_SUCCESS_QUERY } from '../../../../utils/benchmark'; import { setupWebhook } from '../../../../utils/benchmark/webhook-driver'; import { runWebhookThroughputTest } from './webhook-throughput-harness'; @@ -16,7 +17,10 @@ export async function runSingleInstanceWebhookBenchmark(options: { testInfo: TestInfo; baseUrl: string; dimensions: BenchmarkDimensions; + drainTimeoutSeconds?: number; }): Promise { + const drainTimeoutSeconds = + options.drainTimeoutSeconds ?? SINGLE_INSTANCE_WEBHOOK_DRAIN_TIMEOUT_SECONDS; const handle = setupWebhook({ scenario: { nodeCount: 1, @@ -36,14 +40,14 @@ export async function runSingleInstanceWebhookBenchmark(options: { pipelining: 1, warmupSeconds: 0, durationSeconds: SINGLE_INSTANCE_WEBHOOK_DURATION_SECONDS, - timeoutMs: - (SINGLE_INSTANCE_WEBHOOK_DURATION_SECONDS + - SINGLE_INSTANCE_WEBHOOK_DRAIN_TIMEOUT_SECONDS + - 60) * - 1000, - drainTimeoutSeconds: SINGLE_INSTANCE_WEBHOOK_DRAIN_TIMEOUT_SECONDS, + timeoutMs: (SINGLE_INSTANCE_WEBHOOK_DURATION_SECONDS + drainTimeoutSeconds + 60) * 1000, + drainTimeoutSeconds, maxErrorRatePct: 1, - minCompletedResponseRatio: 0.95, + minCompletedResponseRatio: drainTimeoutSeconds === 0 ? undefined : 0.95, + counterReader: + options.api.options.workflowSettings?.engineType === 'v2' + ? undefined + : async () => await options.api.metrics.getCounter(WORKFLOW_SUCCESS_QUERY), dimensions: options.dimensions, }); } diff --git a/packages/testing/playwright/tests/infrastructure/benchmarks/harness/webhook-throughput-harness.ts b/packages/testing/playwright/tests/infrastructure/benchmarks/harness/webhook-throughput-harness.ts index 62f31aa0b0a..f8df58cd82a 100644 --- a/packages/testing/playwright/tests/infrastructure/benchmarks/harness/webhook-throughput-harness.ts +++ b/packages/testing/playwright/tests/infrastructure/benchmarks/harness/webhook-throughput-harness.ts @@ -20,7 +20,7 @@ import { resolveMetricQuery, waitForThroughput, } from '../../../../utils/benchmark'; -import type { BenchmarkDimensions } from '../../../../utils/benchmark'; +import type { BenchmarkDimensions, CompletionCounterReader } from '../../../../utils/benchmark'; import type { WebhookHandle } from '../../../../utils/benchmark/webhook-driver'; import { attachMetric } from '../../../../utils/performance-helper'; @@ -39,8 +39,12 @@ export interface WebhookThroughputOptions { drainTimeoutSeconds?: number; /** PromQL metric to track workflow completions. Defaults to resolveMetricQuery(testInfo). */ metricQuery?: string; + /** Direct completion reader for single-instance comparisons. */ + counterReader?: CompletionCounterReader; /** Additional dimensions attached to every metric and the run report. */ dimensions?: BenchmarkDimensions; + /** Human-readable label for sibling runs in one benchmark summary. */ + variant?: string; /** * Seconds of discarded load BEFORE the measurement window. Warms V8 JIT, * PG connection pool, webhook cache, and BullMQ worker hot paths so the @@ -63,7 +67,7 @@ export interface WebhookThroughputOptions { /** * Runs a webhook throughput test using autocannon for HTTP load generation - * and VictoriaMetrics for workflow completion tracking. + * and the configured counter reader for workflow completion tracking. * * Webhook differs from kafka harnesses: load is generated by autocannon (HTTP) * rather than via a TriggerHandle.publishAtRate. The shared `executeLoad` @@ -95,6 +99,7 @@ export async function runWebhookThroughputTest(options: WebhookThroughputOptions duration_s: durationSeconds, }; if (nodeOutputSize !== undefined) dimensions.output = nodeOutputSize; + if (options.variant !== undefined) dimensions.variant = options.variant; // Warm-up runs inside setupBenchmarkRun so the baseline counter is captured // AFTER the probe POST — otherwise warm-up traffic would offset the measurement. @@ -104,6 +109,7 @@ export async function runWebhookThroughputTest(options: WebhookThroughputOptions testInfo, handle, metricQuery, + counterReader: options.counterReader, createOptions: { webhookPrefix: 'bench' }, warmUp: async ({ webhookPath }) => { if (!webhookPath) throw new Error('Webhook path missing — workflow has no webhook node?'); @@ -141,6 +147,7 @@ export async function runWebhookThroughputTest(options: WebhookThroughputOptions ` Workflow: ${nodeCount} nodes (${nodeOutputSize})`, ); + const loadEndAt = Date.now() + durationSeconds * 1000; const [cannonResult, throughputResult] = await Promise.all([ autocannon({ url: webhookUrl, @@ -161,6 +168,7 @@ export async function runWebhookThroughputTest(options: WebhookThroughputOptions timeoutMs: (durationSeconds + drainTimeoutSeconds) * 1000, baselineValue: setup.baselineCounter, metricQuery, + counterReader: setup.counterReader, }), ]); @@ -267,6 +275,8 @@ export async function runWebhookThroughputTest(options: WebhookThroughputOptions successfulResponses > 0 ? throughputResult.totalCompleted / successfulResponses : 0; expect(completionRatio).toBeGreaterThanOrEqual(options.minCompletedResponseRatio); } else { - expect(throughputResult.totalCompleted).toBeGreaterThan(0); + expect(throughputResult.tailExecPerSec ?? 0).toBeGreaterThan(0); + const lastActive = throughputResult.samples.findLast((sample) => sample.delta > 0); + expect(lastActive?.timestamp ?? 0).toBeGreaterThanOrEqual(loadEndAt - 10_000); } } diff --git a/packages/testing/playwright/tests/infrastructure/benchmarks/kafka/single-instance-ceiling-engine-v2-lazy-cache.spec.ts b/packages/testing/playwright/tests/infrastructure/benchmarks/kafka/single-instance-ceiling-engine-v2-lazy-cache.spec.ts new file mode 100644 index 00000000000..c3680003919 --- /dev/null +++ b/packages/testing/playwright/tests/infrastructure/benchmarks/kafka/single-instance-ceiling-engine-v2-lazy-cache.spec.ts @@ -0,0 +1,48 @@ +import { test } from '../../../../fixtures/base'; +import { BENCHMARK_MAIN_RESOURCES, benchConfig } from '../../../../playwright-projects'; +import { ENGINE_V2_LAZY_CACHE_BENCHMARK_PROFILE } from '../../../../utils/benchmark'; +import { runKafkaBacklogTest } from '../harness/kafka-backlog-harness'; + +const MESSAGE_COUNT = 10_000; + +test.use({ + capability: benchConfig( + `single-instance-ceiling-${ENGINE_V2_LAZY_CACHE_BENCHMARK_PROFILE.isolationSuffix}`, + { + kafka: true, + engine: ENGINE_V2_LAZY_CACHE_BENCHMARK_PROFILE.engineDatabase, + env: ENGINE_V2_LAZY_CACHE_BENCHMARK_PROFILE.env, + }, + ), +}); + +test.describe( + 'How much can execution engine v2 process with lazy expression isolates on a single instance?', + { + tag: '@bench:kafka', + annotation: [ + { type: 'owner', description: 'Catalysts' }, + { + type: 'question', + description: 'single-instance-throughput-ceiling-engine-v2-vm-lazy-cache', + }, + ], + }, + () => { + test('Kafka trigger + 1 noop, 1KB payload, 10k msgs (engine v2, VM lazy + compile cache)', async ({ + api, + services, + }, testInfo) => { + await runKafkaBacklogTest({ + api, + services, + testInfo, + messageCount: MESSAGE_COUNT, + timeoutMs: 600_000, + resourceSummary: { plan: BENCHMARK_MAIN_RESOURCES }, + dimensions: ENGINE_V2_LAZY_CACHE_BENCHMARK_PROFILE.dimensions, + requireTailMeasurement: true, + }); + }); + }, +); diff --git a/packages/testing/playwright/tests/infrastructure/benchmarks/kafka/single-instance-ceiling-engine-v2-split-db.spec.ts b/packages/testing/playwright/tests/infrastructure/benchmarks/kafka/single-instance-ceiling-engine-v2-split-db.spec.ts new file mode 100644 index 00000000000..84f12af864d --- /dev/null +++ b/packages/testing/playwright/tests/infrastructure/benchmarks/kafka/single-instance-ceiling-engine-v2-split-db.spec.ts @@ -0,0 +1,42 @@ +import { test } from '../../../../fixtures/base'; +import { BENCHMARK_MAIN_RESOURCES, benchConfig } from '../../../../playwright-projects'; +import { ENGINE_V2_SPLIT_DB_LAZY_CACHE_BENCHMARK_PROFILE } from '../../../../utils/benchmark'; +import { runKafkaBacklogTest } from '../harness/kafka-backlog-harness'; + +test.use({ + capability: benchConfig( + `single-instance-ceiling-${ENGINE_V2_SPLIT_DB_LAZY_CACHE_BENCHMARK_PROFILE.isolationSuffix}`, + { + kafka: true, + engine: ENGINE_V2_SPLIT_DB_LAZY_CACHE_BENCHMARK_PROFILE.engineDatabase, + env: ENGINE_V2_SPLIT_DB_LAZY_CACHE_BENCHMARK_PROFILE.env, + }, + ), +}); + +test.describe( + 'Engine v2 split database Kafka throughput', + { + annotation: [ + { type: 'owner', description: 'Catalysts' }, + { type: 'question', description: 'engine-v2-split-db-kafka' }, + ], + }, + () => { + test('Kafka trigger + 1 noop, 1KB payload, 10k msgs (engine v2, split DB, VM lazy + cache) @bench:kafka', async ({ + api, + services, + }, testInfo) => { + await runKafkaBacklogTest({ + api, + services, + testInfo, + messageCount: 10_000, + timeoutMs: 600_000, + resourceSummary: { plan: BENCHMARK_MAIN_RESOURCES }, + dimensions: ENGINE_V2_SPLIT_DB_LAZY_CACHE_BENCHMARK_PROFILE.dimensions, + requireTailMeasurement: true, + }); + }); + }, +); diff --git a/packages/testing/playwright/tests/infrastructure/benchmarks/kafka/single-instance-ceiling-engine-v2.spec.ts b/packages/testing/playwright/tests/infrastructure/benchmarks/kafka/single-instance-ceiling-engine-v2.spec.ts new file mode 100644 index 00000000000..a7d76a9cd29 --- /dev/null +++ b/packages/testing/playwright/tests/infrastructure/benchmarks/kafka/single-instance-ceiling-engine-v2.spec.ts @@ -0,0 +1,45 @@ +import { test } from '../../../../fixtures/base'; +import { BENCHMARK_MAIN_RESOURCES, benchConfig } from '../../../../playwright-projects'; +import { ENGINE_V2_BENCHMARK_PROFILE } from '../../../../utils/benchmark'; +import { runKafkaBacklogTest } from '../harness/kafka-backlog-harness'; + +const MESSAGE_COUNT = 5_000; + +test.use({ + capability: benchConfig( + `single-instance-ceiling-${ENGINE_V2_BENCHMARK_PROFILE.isolationSuffix}`, + { + kafka: true, + engine: ENGINE_V2_BENCHMARK_PROFILE.engineDatabase, + env: ENGINE_V2_BENCHMARK_PROFILE.env, + }, + ), +}); + +test.describe( + 'How much can execution engine v2 process on a single instance?', + { + tag: '@bench:kafka', + annotation: [ + { type: 'owner', description: 'Catalysts' }, + { type: 'question', description: 'single-instance-throughput-ceiling-engine-v2' }, + ], + }, + () => { + test('Kafka trigger + 1 noop, 1KB payload, 5k msgs (engine v2)', async ({ + api, + services, + }, testInfo) => { + await runKafkaBacklogTest({ + api, + services, + testInfo, + messageCount: MESSAGE_COUNT, + timeoutMs: 300_000, + resourceSummary: { plan: BENCHMARK_MAIN_RESOURCES }, + dimensions: ENGINE_V2_BENCHMARK_PROFILE.dimensions, + requireTailMeasurement: true, + }); + }); + }, +); diff --git a/packages/testing/playwright/tests/infrastructure/benchmarks/kafka/single-instance-ceiling-lazy-cache.spec.ts b/packages/testing/playwright/tests/infrastructure/benchmarks/kafka/single-instance-ceiling-lazy-cache.spec.ts index 5ac05b34dfb..3951ac4ad17 100644 --- a/packages/testing/playwright/tests/infrastructure/benchmarks/kafka/single-instance-ceiling-lazy-cache.spec.ts +++ b/packages/testing/playwright/tests/infrastructure/benchmarks/kafka/single-instance-ceiling-lazy-cache.spec.ts @@ -3,7 +3,7 @@ import { BENCHMARK_MAIN_RESOURCES, benchConfig } from '../../../../playwright-pr import { VM_LAZY_CACHE_BENCHMARK_PROFILE } from '../../../../utils/benchmark'; import { runKafkaBacklogTest } from '../harness/kafka-backlog-harness'; -const MESSAGE_COUNT = 2_000; +const MESSAGE_COUNT = 20_000; test.use({ capability: benchConfig( @@ -22,7 +22,7 @@ test.describe( ], }, () => { - test('Kafka trigger + 1 noop, 1KB payload, 2k msgs (VM lazy + compile cache)', async ({ + test('Kafka trigger + 1 noop, 1KB payload, 20k msgs (VM lazy + compile cache)', async ({ api, services, }, testInfo) => { @@ -31,7 +31,7 @@ test.describe( services, testInfo, messageCount: MESSAGE_COUNT, - timeoutMs: 180_000, + timeoutMs: 600_000, resourceSummary: { plan: BENCHMARK_MAIN_RESOURCES }, minimumCompletionRatio: 1, dimensions: VM_LAZY_CACHE_BENCHMARK_PROFILE.dimensions, diff --git a/packages/testing/playwright/tests/infrastructure/benchmarks/kafka/single-instance-ceiling.spec.ts b/packages/testing/playwright/tests/infrastructure/benchmarks/kafka/single-instance-ceiling.spec.ts index 9524c9d4fd7..b73de5e94f8 100644 --- a/packages/testing/playwright/tests/infrastructure/benchmarks/kafka/single-instance-ceiling.spec.ts +++ b/packages/testing/playwright/tests/infrastructure/benchmarks/kafka/single-instance-ceiling.spec.ts @@ -3,7 +3,7 @@ import { BENCHMARK_MAIN_RESOURCES, benchConfig } from '../../../../playwright-pr import { VM_EAGER_BENCHMARK_PROFILE } from '../../../../utils/benchmark'; import { runKafkaBacklogTest } from '../harness/kafka-backlog-harness'; -const MESSAGE_COUNT = 2_000; +const MESSAGE_COUNT = 5_000; test.use({ capability: benchConfig(`single-instance-ceiling-${VM_EAGER_BENCHMARK_PROFILE.isolationSuffix}`, { @@ -22,13 +22,13 @@ test.describe( ], }, () => { - test('Kafka trigger + 1 noop, 1KB payload, 2k msgs', async ({ api, services }, testInfo) => { + test('Kafka trigger + 1 noop, 1KB payload, 5k msgs', async ({ api, services }, testInfo) => { await runKafkaBacklogTest({ api, services, testInfo, messageCount: MESSAGE_COUNT, - timeoutMs: 180_000, + timeoutMs: 300_000, resourceSummary: { plan: BENCHMARK_MAIN_RESOURCES }, minimumCompletionRatio: 1, dimensions: VM_EAGER_BENCHMARK_PROFILE.dimensions, diff --git a/packages/testing/playwright/tests/infrastructure/benchmarks/webhook/webhook-single-instance-engine-v2-lazy-cache.spec.ts b/packages/testing/playwright/tests/infrastructure/benchmarks/webhook/webhook-single-instance-engine-v2-lazy-cache.spec.ts new file mode 100644 index 00000000000..ea6a8c1e068 --- /dev/null +++ b/packages/testing/playwright/tests/infrastructure/benchmarks/webhook/webhook-single-instance-engine-v2-lazy-cache.spec.ts @@ -0,0 +1,48 @@ +import { test } from '../../../../fixtures/base'; +import { benchConfig } from '../../../../playwright-projects'; +import { ENGINE_V2_LAZY_CACHE_BENCHMARK_PROFILE } from '../../../../utils/benchmark'; +import { + runSingleInstanceWebhookBenchmark, + SINGLE_INSTANCE_WEBHOOK_CONNECTIONS, + SINGLE_INSTANCE_WEBHOOK_DURATION_SECONDS, +} from '../harness/single-instance-webhook-harness'; + +test.use({ + capability: benchConfig( + `webhook-single-instance-${ENGINE_V2_LAZY_CACHE_BENCHMARK_PROFILE.isolationSuffix}`, + { + engine: ENGINE_V2_LAZY_CACHE_BENCHMARK_PROFILE.engineDatabase, + env: ENGINE_V2_LAZY_CACHE_BENCHMARK_PROFILE.env, + }, + ), +}); + +test.describe( + 'What is the single-instance webhook ingestion ceiling with execution engine v2 and lazy expression isolates?', + { + tag: '@bench:webhook', + annotation: [ + { type: 'owner', description: 'Catalysts' }, + { + type: 'question', + description: 'webhook-single-instance-engine-v2-vm-lazy-cache', + }, + ], + }, + () => { + test(`Async webhook + 1 noop, 1KB payload, ${SINGLE_INSTANCE_WEBHOOK_CONNECTIONS} connections × ${SINGLE_INSTANCE_WEBHOOK_DURATION_SECONDS}s (1 main, no workers, engine v2, VM lazy + compile cache)`, async ({ + api, + services, + backendUrl, + }, testInfo) => { + await runSingleInstanceWebhookBenchmark({ + api, + services, + testInfo, + baseUrl: backendUrl, + dimensions: ENGINE_V2_LAZY_CACHE_BENCHMARK_PROFILE.dimensions, + drainTimeoutSeconds: 0, + }); + }); + }, +); diff --git a/packages/testing/playwright/tests/infrastructure/benchmarks/webhook/webhook-single-instance-engine-v2-split-db.spec.ts b/packages/testing/playwright/tests/infrastructure/benchmarks/webhook/webhook-single-instance-engine-v2-split-db.spec.ts new file mode 100644 index 00000000000..6f175f9b142 --- /dev/null +++ b/packages/testing/playwright/tests/infrastructure/benchmarks/webhook/webhook-single-instance-engine-v2-split-db.spec.ts @@ -0,0 +1,44 @@ +import { test } from '../../../../fixtures/base'; +import { benchConfig } from '../../../../playwright-projects'; +import { ENGINE_V2_SPLIT_DB_LAZY_CACHE_BENCHMARK_PROFILE } from '../../../../utils/benchmark'; +import { + runSingleInstanceWebhookBenchmark, + SINGLE_INSTANCE_WEBHOOK_CONNECTIONS, + SINGLE_INSTANCE_WEBHOOK_DURATION_SECONDS, +} from '../harness/single-instance-webhook-harness'; + +test.use({ + capability: benchConfig( + `webhook-single-instance-${ENGINE_V2_SPLIT_DB_LAZY_CACHE_BENCHMARK_PROFILE.isolationSuffix}`, + { + engine: ENGINE_V2_SPLIT_DB_LAZY_CACHE_BENCHMARK_PROFILE.engineDatabase, + env: ENGINE_V2_SPLIT_DB_LAZY_CACHE_BENCHMARK_PROFILE.env, + }, + ), +}); + +test.describe( + 'Engine v2 split database webhook throughput', + { + annotation: [ + { type: 'owner', description: 'Catalysts' }, + { type: 'question', description: 'engine-v2-split-db-webhook' }, + ], + }, + () => { + test(`Async webhook + 1 noop, 1KB payload, ${SINGLE_INSTANCE_WEBHOOK_CONNECTIONS} connections × ${SINGLE_INSTANCE_WEBHOOK_DURATION_SECONDS}s (engine v2, split DB, VM lazy + cache) @bench:webhook`, async ({ + api, + services, + backendUrl, + }, testInfo) => { + await runSingleInstanceWebhookBenchmark({ + api, + services, + testInfo, + baseUrl: backendUrl, + dimensions: ENGINE_V2_SPLIT_DB_LAZY_CACHE_BENCHMARK_PROFILE.dimensions, + drainTimeoutSeconds: 0, + }); + }); + }, +); diff --git a/packages/testing/playwright/tests/infrastructure/benchmarks/webhook/webhook-single-instance-engine-v2.spec.ts b/packages/testing/playwright/tests/infrastructure/benchmarks/webhook/webhook-single-instance-engine-v2.spec.ts new file mode 100644 index 00000000000..43c3f45c36b --- /dev/null +++ b/packages/testing/playwright/tests/infrastructure/benchmarks/webhook/webhook-single-instance-engine-v2.spec.ts @@ -0,0 +1,45 @@ +import { test } from '../../../../fixtures/base'; +import { benchConfig } from '../../../../playwright-projects'; +import { ENGINE_V2_BENCHMARK_PROFILE } from '../../../../utils/benchmark'; +import { + runSingleInstanceWebhookBenchmark, + SINGLE_INSTANCE_WEBHOOK_CONNECTIONS, + SINGLE_INSTANCE_WEBHOOK_DURATION_SECONDS, +} from '../harness/single-instance-webhook-harness'; + +test.use({ + capability: benchConfig( + `webhook-single-instance-${ENGINE_V2_BENCHMARK_PROFILE.isolationSuffix}`, + { + engine: ENGINE_V2_BENCHMARK_PROFILE.engineDatabase, + env: ENGINE_V2_BENCHMARK_PROFILE.env, + }, + ), +}); + +test.describe( + 'What is the single-instance webhook ingestion ceiling with execution engine v2?', + { + tag: '@bench:webhook', + annotation: [ + { type: 'owner', description: 'Catalysts' }, + { type: 'question', description: 'webhook-single-instance-engine-v2' }, + ], + }, + () => { + test(`Async webhook + 1 noop, 1KB payload, ${SINGLE_INSTANCE_WEBHOOK_CONNECTIONS} connections × ${SINGLE_INSTANCE_WEBHOOK_DURATION_SECONDS}s (1 main, no workers, engine v2)`, async ({ + api, + services, + backendUrl, + }, testInfo) => { + await runSingleInstanceWebhookBenchmark({ + api, + services, + testInfo, + baseUrl: backendUrl, + dimensions: ENGINE_V2_BENCHMARK_PROFILE.dimensions, + drainTimeoutSeconds: 0, + }); + }); + }, +); diff --git a/packages/testing/playwright/tests/infrastructure/benchmarks/webhook/webhook-sync-latency-floor.spec.ts b/packages/testing/playwright/tests/infrastructure/benchmarks/webhook/webhook-sync-latency-floor.spec.ts index 9bbef31008e..a73a9742e40 100644 --- a/packages/testing/playwright/tests/infrastructure/benchmarks/webhook/webhook-sync-latency-floor.spec.ts +++ b/packages/testing/playwright/tests/infrastructure/benchmarks/webhook/webhook-sync-latency-floor.spec.ts @@ -58,6 +58,7 @@ test.describe( testInfo, baseUrl: backendUrl, connections, + variant: `${connections} connection${connections === 1 ? '' : 's'}`, pipelining: 1, warmupSeconds: WARMUP_SECONDS, durationSeconds: DURATION_SECONDS, diff --git a/packages/testing/playwright/utils/benchmark/expression-benchmark-profiles.ts b/packages/testing/playwright/utils/benchmark/expression-benchmark-profiles.ts index 31c026cc784..afdca92f64e 100644 --- a/packages/testing/playwright/utils/benchmark/expression-benchmark-profiles.ts +++ b/packages/testing/playwright/utils/benchmark/expression-benchmark-profiles.ts @@ -1,12 +1,13 @@ import type { BenchmarkDimensions } from './types'; -interface ExpressionBenchmarkProfile { +interface RuntimeBenchmarkProfile { isolationSuffix: string; env: Record; dimensions: BenchmarkDimensions; + engineDatabase?: 'shared' | 'split'; } -export const VM_EAGER_BENCHMARK_PROFILE: ExpressionBenchmarkProfile = { +export const VM_EAGER_BENCHMARK_PROFILE: RuntimeBenchmarkProfile = { isolationSuffix: 'vm-eager', env: { N8N_EXPRESSION_ENGINE: 'vm', @@ -14,6 +15,8 @@ export const VM_EAGER_BENCHMARK_PROFILE: ExpressionBenchmarkProfile = { N8N_EXPRESSION_ENGINE_COMPILE_CACHE: 'false', }, dimensions: { + runtime_profile: 'v1 eager', + execution_engine: 'v1', expression_engine: 'vm', expression_lazy_acquire: 0, expression_compile_cache: 0, @@ -21,7 +24,7 @@ export const VM_EAGER_BENCHMARK_PROFILE: ExpressionBenchmarkProfile = { }, }; -export const VM_LAZY_CACHE_BENCHMARK_PROFILE: ExpressionBenchmarkProfile = { +export const VM_LAZY_CACHE_BENCHMARK_PROFILE: RuntimeBenchmarkProfile = { isolationSuffix: 'vm-lazy-cache', env: { N8N_EXPRESSION_ENGINE: 'vm', @@ -29,9 +32,56 @@ export const VM_LAZY_CACHE_BENCHMARK_PROFILE: ExpressionBenchmarkProfile = { N8N_EXPRESSION_ENGINE_COMPILE_CACHE: 'true', }, dimensions: { + runtime_profile: 'v1 lazy/cache', + execution_engine: 'v1', expression_engine: 'vm', expression_lazy_acquire: 1, expression_compile_cache: 1, expression_profile: 'vm-lazy-cache', }, }; + +export const ENGINE_V2_BENCHMARK_PROFILE: RuntimeBenchmarkProfile = { + isolationSuffix: 'engine-v2', + env: { + ...VM_EAGER_BENCHMARK_PROFILE.env, + }, + engineDatabase: 'shared', + dimensions: { + runtime_profile: 'v2 eager', + execution_engine: 'v2', + database_topology: 'shared-postgres', + database_metrics_scope: 'combined', + expression_engine: 'vm', + expression_lazy_acquire: 0, + expression_compile_cache: 0, + expression_profile: 'vm-eager', + }, +}; + +export const ENGINE_V2_LAZY_CACHE_BENCHMARK_PROFILE: RuntimeBenchmarkProfile = { + isolationSuffix: 'engine-v2-vm-lazy-cache', + env: { + ...VM_LAZY_CACHE_BENCHMARK_PROFILE.env, + }, + engineDatabase: 'shared', + dimensions: { + ...VM_LAZY_CACHE_BENCHMARK_PROFILE.dimensions, + runtime_profile: 'v2 lazy/cache', + execution_engine: 'v2', + database_topology: 'shared-postgres', + database_metrics_scope: 'combined', + }, +}; + +export const ENGINE_V2_SPLIT_DB_LAZY_CACHE_BENCHMARK_PROFILE: RuntimeBenchmarkProfile = { + ...ENGINE_V2_LAZY_CACHE_BENCHMARK_PROFILE, + isolationSuffix: 'engine-v2-split-db-vm-lazy-cache', + engineDatabase: 'split', + dimensions: { + ...ENGINE_V2_LAZY_CACHE_BENCHMARK_PROFILE.dimensions, + runtime_profile: 'v2 lazy/cache, split DB', + database_topology: 'split-postgres', + database_metrics_scope: 'control-plane', + }, +}; diff --git a/packages/testing/playwright/utils/benchmark/load-executors.ts b/packages/testing/playwright/utils/benchmark/load-executors.ts index e5e47d4a712..020c3cf61ad 100644 --- a/packages/testing/playwright/utils/benchmark/load-executors.ts +++ b/packages/testing/playwright/utils/benchmark/load-executors.ts @@ -11,7 +11,7 @@ import type { MetricsHelper } from 'n8n-containers'; import { measureStageWindows, measureSteadyPhases, waitForThroughput } from './throughput-measure'; -import type { ThroughputResult } from './throughput-measure'; +import type { CompletionCounterReader, ThroughputResult } from './throughput-measure'; import type { LoadProfile, TriggerHandle } from './types'; /** Context shared by every executor — metrics + measurement plumbing, no load specifics. */ @@ -20,6 +20,7 @@ export interface ExecutorContext { metrics: MetricsHelper; baselineCounter: number; metricQuery: string; + counterReader?: CompletionCounterReader; timeoutMs: number; nodeCount: number; } @@ -59,6 +60,7 @@ async function runPreloaded(load: PreloadedLoad, ctx: ExecutorContext): Promise< timeoutMs: ctx.timeoutMs, baselineValue: ctx.baselineCounter, metricQuery: ctx.metricQuery, + counterReader: ctx.counterReader, }); return { @@ -91,6 +93,7 @@ async function runSteady(load: SteadyLoad, ctx: ExecutorContext): Promise { const byScale = new Map(); for (const { path, report } of reports) { - // The sizing matrix represents the default runtime. Optimized comparison - // profiles remain available in benchmark metrics and run-report artifacts. - if (report.scenario.dimensions['expression_profile'] === 'vm-lazy-cache') continue; + // The sizing matrix represents the default runtime. Comparison profiles + // remain available in benchmark metrics and run-report artifacts. + if ( + report.scenario.dimensions['expression_profile'] === 'vm-lazy-cache' || + report.scenario.dimensions['execution_engine'] === 'v2' + ) { + continue; + } const cellCoord = findMapping(path, report, mapping); if (!cellCoord) { diff --git a/packages/testing/playwright/utils/benchmark/throughput-measure.ts b/packages/testing/playwright/utils/benchmark/throughput-measure.ts index 68402bd02eb..b867207f6bd 100644 --- a/packages/testing/playwright/utils/benchmark/throughput-measure.ts +++ b/packages/testing/playwright/utils/benchmark/throughput-measure.ts @@ -212,6 +212,7 @@ export function measureSteadyPhases( export const WORKFLOW_SUCCESS_QUERY = 'n8n_workflow_success_total'; export const QUEUE_JOBS_COMPLETED_QUERY = 'n8n_scaling_mode_queue_jobs_completed'; +export type CompletionCounterReader = () => Promise; /** * Returns the completion metric for the current Playwright project. @@ -257,6 +258,7 @@ export async function waitForThroughput( * per-stage measurements so a ramp test can identify the breaking point. */ stageBoundaries?: number[]; + counterReader?: CompletionCounterReader; }, ): Promise { const { @@ -269,6 +271,7 @@ export async function waitForThroughput( stallThresholdMs = 60_000, publishEndAt, stageBoundaries, + counterReader, } = options; const samples: ThroughputSample[] = []; @@ -282,9 +285,14 @@ export async function waitForThroughput( const remaining = deadline - Date.now(); await new Promise((resolve) => setTimeout(resolve, Math.min(pollIntervalMs, remaining))); - let results; + let current: number; try { - results = await metrics.query(`sum(last_over_time(${metricQuery}[5m]))`); + if (counterReader) { + current = await counterReader(); + } else { + const results = await metrics.query(`sum(last_over_time(${metricQuery}[5m]))`); + current = results.length > 0 ? results.reduce((sum, r) => sum + r.value, 0) : 0; + } } catch (error) { console.log( `[THROUGHPUT] Query error: ${error instanceof Error ? error.message : String(error)}`, @@ -292,8 +300,6 @@ export async function waitForThroughput( continue; } - const current = results.length > 0 ? results.reduce((sum, r) => sum + r.value, 0) : 0; - // Monotonic guard: counters should never decrease. // If VictoriaMetrics returns a stale/missing value, skip this sample. if (current < highWaterMark) { @@ -346,7 +352,9 @@ export async function waitForThroughput( export async function getBaselineCounter( metrics: MetricsHelper, metricQuery: string = WORKFLOW_SUCCESS_QUERY, + counterReader?: CompletionCounterReader, ): Promise { + if (counterReader) return await counterReader(); try { const results = await metrics.query(`sum(last_over_time(${metricQuery}[5m]))`); return results.length > 0 ? results.reduce((sum, r) => sum + r.value, 0) : 0; From 7b059d64f13a3c40416ac82a3c757f41433ee190 Mon Sep 17 00:00:00 2001 From: James Martin <393605+ymhr@users.noreply.github.com> Date: Fri, 18 Sep 2026 08:50:32 +0000 Subject: [PATCH 02/67] fix(editor): Split global variable scopes to be added to custom roles separately (#38724) --- .../__tests__/custom-role-scopes.test.ts | 31 ++++++++ .../src/roles/custom-role-scopes.ee.ts | 17 ++++- .../cli/test/integration/variables.test.ts | 73 +++++++++++++++++++ .../frontend/@n8n/i18n/src/locales/en.json | 3 + .../roles/instance/instanceRoleScopes.test.ts | 20 +++++ .../roles/instance/instanceRoleScopes.ts | 6 ++ 6 files changed, 148 insertions(+), 2 deletions(-) diff --git a/packages/@n8n/permissions/src/roles/__tests__/custom-role-scopes.test.ts b/packages/@n8n/permissions/src/roles/__tests__/custom-role-scopes.test.ts index 97f14e95c47..5a130d3a742 100644 --- a/packages/@n8n/permissions/src/roles/__tests__/custom-role-scopes.test.ts +++ b/packages/@n8n/permissions/src/roles/__tests__/custom-role-scopes.test.ts @@ -117,6 +117,37 @@ describe('custom role scope whitelists', () => { expect(manage.length).toBeGreaterThan(view.length); }); + it('exposes "Variables: View" as exactly the global variable list/read pair', () => { + expect(GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS.variable.View).toEqual([ + 'variable:list', + 'variable:read', + ]); + }); + + it('keeps "Variables: View" within GLOBAL_MEMBER_SCOPES', () => { + // A custom role built to mirror Member must never end up with more than Member. + for (const scope of GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS.variable.View) { + expect(GLOBAL_MEMBER_SCOPES).toContain(scope); + } + }); + + it('keeps variable "Manage" a strict superset of variable "View"', () => { + // The editor's implied/downgrade arithmetic (SUPERSEDED_BY: View -> Manage) + // only holds while Manage contains everything View grants. + const view = GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS.variable.View; + const manage = GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS.variable.Manage; + expect(view.every((scope) => (manage as readonly string[]).includes(scope))).toBe(true); + expect(manage.length).toBeGreaterThan(view.length); + }); + + it('keeps variable scopes out of the settings.Manage bundle', () => { + // Global variables have their own group, so granting instance Settings no + // longer grants them. Every scope must live under exactly one group. + const bundle = GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS.settings.Manage as readonly string[]; + expect(bundle).not.toContain('variable:list'); + expect(bundle).not.toContain('variable:read'); + }); + it('keeps "Tags: View" within GLOBAL_MEMBER_SCOPES', () => { // "Tags: View" is granted to every instance role by default (see // instanceRoleScopes.ts), so it must never exceed what the built-in Member diff --git a/packages/@n8n/permissions/src/roles/custom-role-scopes.ee.ts b/packages/@n8n/permissions/src/roles/custom-role-scopes.ee.ts index 3d6adfcef65..17f36143d7a 100644 --- a/packages/@n8n/permissions/src/roles/custom-role-scopes.ee.ts +++ b/packages/@n8n/permissions/src/roles/custom-role-scopes.ee.ts @@ -100,8 +100,6 @@ export const GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS = { 'eventBusDestination:delete', 'eventBusDestination:list', 'eventBusDestination:test', - 'variable:list', - 'variable:read', 'dataTable:list', 'aiPreference:create', // Context (instance-wide AI preferences) 'aiPreference:read', @@ -169,6 +167,21 @@ export const GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS = { View: ['tag:read', 'tag:list'], Manage: ['tag:read', 'tag:list', 'tag:create', 'tag:update', 'tag:delete'], }, + variable: { + // Global (instance-level) variables only; project variables are granted per + // project via `projectVariable:*` on a project role. View mirrors + // GLOBAL_MEMBER_SCOPES exactly, so a custom role built to match Member never + // exceeds it. The list response carries each variable's value, so View reads + // values and is not only discovery — hence it is opt-in, not mandatory. + View: ['variable:list', 'variable:read'], + Manage: [ + 'variable:list', + 'variable:read', + 'variable:create', + 'variable:update', + 'variable:delete', + ], + }, project: { Create: ['project:create'], }, diff --git a/packages/cli/test/integration/variables.test.ts b/packages/cli/test/integration/variables.test.ts index fbe89ef9684..bc1a3251b46 100644 --- a/packages/cli/test/integration/variables.test.ts +++ b/packages/cli/test/integration/variables.test.ts @@ -10,6 +10,7 @@ import { getVariableByKey, } from '@test-integration/db/variables'; +import { createCustomRoleWithScopeSlugs } from './shared/db/roles'; import { createOwner, createUser } from './shared/db/users'; import type { SuperAgentTest } from './shared/types'; import * as utils from './shared/utils/'; @@ -119,6 +120,78 @@ describe('GET /variables/:id', () => { }); }); +// ---------------------------------------- +// Custom instance roles - global variables +// ---------------------------------------- +describe('GET /variables - custom instance roles', () => { + // A custom instance role only sees global variables when it holds `variable:list`. + // `variable:list` is now its own permission option, so a role can read global + // variables without also holding every instance settings scope. + let authNoVariableScopesAgent: SuperAgentTest; + let authVariableViewAgent: SuperAgentTest; + let globalVariable: Variables; + let projectVariable: Variables; + + beforeAll(async () => { + const roleWithoutVariableScopes = await createCustomRoleWithScopeSlugs(['user:list'], { + roleType: 'global', + }); + const roleWithVariableView = await createCustomRoleWithScopeSlugs( + ['user:list', 'variable:list', 'variable:read'], + { roleType: 'global' }, + ); + + const userWithoutVariableScopes = await createUser({ role: roleWithoutVariableScopes }); + const userWithVariableView = await createUser({ role: roleWithVariableView }); + await Promise.all([ + linkUserToProject(userWithoutVariableScopes, project, 'project:admin'), + linkUserToProject(userWithVariableView, project, 'project:admin'), + ]); + + authNoVariableScopesAgent = testServer.authAgentFor(userWithoutVariableScopes); + authVariableViewAgent = testServer.authAgentFor(userWithVariableView); + }); + + beforeEach(async () => { + [globalVariable, projectVariable] = await Promise.all([ + createVariable('globalVar', 'globalValue'), + createProjectVariable('projectVar', 'projectValue', project), + ]); + }); + + test('should filter out global variables for a role without variable:list', async () => { + const response = await authNoVariableScopesAgent.get('/variables'); + + // Filtered out silently, not a 403 - the list still returns the project variables. + expect(response.statusCode).toBe(200); + expect(response.body.data.map((variable: Variables) => variable.key)).toEqual([ + projectVariable.key, + ]); + }); + + test('should return global variables for a role with variable:list', async () => { + const response = await authVariableViewAgent.get('/variables'); + + expect(response.statusCode).toBe(200); + expect(response.body.data.map((variable: Variables) => variable.key).sort()).toEqual( + [globalVariable.key, projectVariable.key].sort(), + ); + }); + + test('should return a single global variable for a role with variable:read', async () => { + const response = await authVariableViewAgent.get(`/variables/${globalVariable.id}`); + + expect(response.statusCode).toBe(200); + expect(response.body.data.key).toBe(globalVariable.key); + }); + + test('should deny a single global variable to a role without variable:read', async () => { + const response = await authNoVariableScopesAgent.get(`/variables/${globalVariable.id}`); + + expect(response.statusCode).toBe(403); + }); +}); + // ---------------------------------------- // POST /variables - create a new variable // ---------------------------------------- diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index f82df500c12..39ce62e2e61 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -3030,6 +3030,7 @@ "instanceRoles.resource.role": "Roles", "instanceRoles.resource.apiKey": "API keys", "instanceRoles.resource.tag": "Tags", + "instanceRoles.resource.variable": "Variables", "instanceRoles.resource.project": "Projects", "instanceRoles.resource.insights": "Insights", "instanceRoles.option.view": "View", @@ -3059,6 +3060,8 @@ "instanceRoles.description.apiKey.manageAll": "View and delete other users' API keys.", "instanceRoles.description.tag.view": "Users always see and can select any tags associated with workflows they have access to.", "instanceRoles.description.tag.manage": "Create, edit, and delete tags", + "instanceRoles.description.variable.view": "View global variables and their values", + "instanceRoles.description.variable.manage": "Create, edit, and delete global variables", "instanceRoles.description.project.create": "Create new projects", "instanceRoles.description.insights.view": "View instance-level insights", "instanceRoles.warning.manageMembers": "A user with this permission can give themselves or an account they invite permissions they don't currently hold. {link}", diff --git a/packages/frontend/editor-ui/src/features/roles/instance/instanceRoleScopes.test.ts b/packages/frontend/editor-ui/src/features/roles/instance/instanceRoleScopes.test.ts index 41dc30dbaa3..a8aa5169eed 100644 --- a/packages/frontend/editor-ui/src/features/roles/instance/instanceRoleScopes.test.ts +++ b/packages/frontend/editor-ui/src/features/roles/instance/instanceRoleScopes.test.ts @@ -82,6 +82,7 @@ describe('instanceRoleScopes config', () => { it('exposes the configured option labels per resource', () => { expect(Object.keys(INSTANCE_SCOPE_GROUPS.apiKey)).toEqual(['Manage own', 'Manage all']); expect(Object.keys(INSTANCE_SCOPE_GROUPS.tag)).toEqual(['View', 'Manage']); + expect(Object.keys(INSTANCE_SCOPE_GROUPS.variable)).toEqual(['View', 'Manage']); expect(Object.keys(INSTANCE_SCOPE_GROUPS.role)).toEqual(['Manage project roles', 'Manage']); expect(Object.keys(INSTANCE_SCOPE_GROUPS.project)).toEqual(['Create']); expect(Object.keys(INSTANCE_SCOPE_GROUPS.settings)).toEqual([ @@ -154,6 +155,14 @@ describe('isOptionImplied', () => { it('returns false when the superseding option is present but "Manage own" is checked via own scopes only', () => { expect(isOptionImplied(manageOwn, apiKeyGroup.options, ownScopes)).toBe(false); }); + + it('renders variable "View" as implied under a fully-checked variable "Manage"', () => { + const variableGroup = INSTANCE_SCOPE_GROUP_LIST.find((g) => g.resource === 'variable')!; + const view = variableGroup.options.find((o) => o.key === 'View')!; + const manage = variableGroup.options.find((o) => o.key === 'Manage')!; + expect(isOptionImplied(view, variableGroup.options, [...manage.scopes])).toBe(true); + expect(isOptionImplied(view, variableGroup.options, [...view.scopes])).toBe(false); + }); }); describe('resolveOptionState', () => { @@ -306,6 +315,17 @@ describe('toggleOptionInGroup', () => { expect(result).toEqual(['user:read']); }); + it('downgrades to "View" when unchecking variable "Manage"', () => { + const variableGroup = INSTANCE_SCOPE_GROUP_LIST.find((g) => g.resource === 'variable')!; + const manageVariables = variableGroup.options.find((o) => o.key === 'Manage')!; + const result = toggleOptionInGroup( + ['user:list', ...manageVariables.scopes], + manageVariables, + variableGroup.options, + ); + expect(result).toEqual(['user:list', 'variable:list', 'variable:read']); + }); + it('downgrades to "View" when unchecking tag "Manage", keeping the mandatory scopes', () => { const tagGroup = INSTANCE_SCOPE_GROUP_LIST.find((g) => g.resource === 'tag')!; const manageTags = tagGroup.options.find((o) => o.key === 'Manage')!; diff --git a/packages/frontend/editor-ui/src/features/roles/instance/instanceRoleScopes.ts b/packages/frontend/editor-ui/src/features/roles/instance/instanceRoleScopes.ts index 5e0a5ab1bfd..25a385b65d3 100644 --- a/packages/frontend/editor-ui/src/features/roles/instance/instanceRoleScopes.ts +++ b/packages/frontend/editor-ui/src/features/roles/instance/instanceRoleScopes.ts @@ -20,6 +20,7 @@ export const INSTANCE_RESOURCE_ORDER: InstanceResource[] = [ 'role', 'apiKey', 'tag', + 'variable', 'project', 'insights', ]; @@ -31,6 +32,7 @@ export const INSTANCE_RESOURCE_LABEL_KEYS: Record role: 'instanceRoles.resource.role', apiKey: 'instanceRoles.resource.apiKey', tag: 'instanceRoles.resource.tag', + variable: 'instanceRoles.resource.variable', project: 'instanceRoles.resource.project', insights: 'instanceRoles.resource.insights', }; @@ -95,6 +97,10 @@ export const INSTANCE_OPTION_DESCRIPTION_KEYS: Partial< View: 'instanceRoles.description.tag.view', Manage: 'instanceRoles.description.tag.manage', }, + variable: { + View: 'instanceRoles.description.variable.view', + Manage: 'instanceRoles.description.variable.manage', + }, project: { Create: 'instanceRoles.description.project.create' }, insights: { View: 'instanceRoles.description.insights.view' }, }; From 1b39c1d57d681ac776d56c184ec5c7e866002bce Mon Sep 17 00:00:00 2001 From: Ricardo Espinoza Date: Fri, 18 Sep 2026 08:56:41 +0000 Subject: [PATCH 03/67] feat(core): Let the preferences MCP tool read one project (no-changelog) (#39000) --- .../get-user-preferences.tool.test.ts | 94 ++++++++++++++++++- .../mcp/tools/get-user-preferences.tool.ts | 28 +++++- .../__tests__/ai-preference.service.test.ts | 86 +++++++++++++++++ .../cli/src/services/ai-preference.service.ts | 13 +++ 4 files changed, 212 insertions(+), 9 deletions(-) diff --git a/packages/cli/src/modules/mcp/__tests__/get-user-preferences.tool.test.ts b/packages/cli/src/modules/mcp/__tests__/get-user-preferences.tool.test.ts index 915ba538199..3bebb49dd7c 100644 --- a/packages/cli/src/modules/mcp/__tests__/get-user-preferences.tool.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/get-user-preferences.tool.test.ts @@ -20,9 +20,12 @@ const saved = (...texts: string[]) => texts.map((content) => ({ id: `id-${conten const DESCRIPTION = [ 'Returns the preferences saved for this n8n instance, the caller, and their projects: node and credential choices, naming, how work is organised, and patterns to avoid.', 'Call this before you create or modify anything in n8n — a workflow, an Agent, a data table, a folder — and apply what it returns to every change you make for the remainder of the task, not only the first one. If a preference conflicts with something the user asks for directly, follow the user and say which preference you set aside.', + 'When you work inside one project, pass its `projectId` to leave the other projects out.', ].join('\n\n'); const NOTHING_SAVED = 'No preferences are saved for this instance, for you, or for your projects.'; +const NOTHING_SAVED_FOR_PROJECT = + 'No preferences are saved for this instance, for you, or for this project.'; const userWithScopes = (scopeSlugs: string[]) => Object.assign(new User(), { @@ -33,9 +36,12 @@ const userWithScopes = (scopeSlugs: string[]) => const empty: ApplicableAiPreferences = { instance: [], user: [], projects: [] }; const createMocks = (result: ApplicableAiPreferences | Error = empty) => { - const getApplicableAcrossProjects = + const read = () => result instanceof Error ? vi.fn().mockRejectedValue(result) : vi.fn().mockResolvedValue(result); - const aiPreferenceService = mockInstance(AiPreferenceService, { getApplicableAcrossProjects }); + const aiPreferenceService = mockInstance(AiPreferenceService, { + getApplicableAcrossProjects: read(), + getApplicableForProject: read(), + }); const telemetry = mockInstance(Telemetry, { track: vi.fn() }); return { aiPreferenceService, telemetry }; }; @@ -65,12 +71,15 @@ describe('get-user-preferences MCP tool', () => { expect(tool.config.description).toBe(DESCRIPTION); }); - test('takes no arguments, because the caller has nothing to choose', () => { + test('takes only the optional projectId, so a plain call still reads everything', () => { const { aiPreferenceService, telemetry } = createMocks(); const tool = createGetUserPreferencesTool(user, aiPreferenceService, telemetry); - expect(tool.config.inputSchema).toEqual({}); + expect(Object.keys(tool.config.inputSchema!)).toEqual(['projectId']); + expect(tool.config.inputSchema!.projectId.safeParse(undefined).success).toBe(true); + expect(tool.config.inputSchema!.projectId.safeParse('p-1').success).toBe(true); + expect(tool.config.inputSchema!.projectId.safeParse('').success).toBe(false); }); test('is annotated read-only', () => { @@ -153,6 +162,55 @@ describe('get-user-preferences MCP tool', () => { expect(aiPreferenceService.getApplicableAcrossProjects).toHaveBeenCalledTimes(2); }); + describe('narrowed to one project (CONTEXT-136)', () => { + test('reads the one project instead of every project', async () => { + const { aiPreferenceService, telemetry } = createMocks({ + instance: saved('Use British English.'), + user: saved('Keep replies short.'), + projects: [{ id: 'p-1', name: 'Marketing', items: saved('Prefer HubSpot nodes.') }], + }); + const tool = createGetUserPreferencesTool(user, aiPreferenceService, telemetry); + + const result = await tool.handler({ projectId: 'p-1' }); + + expect(aiPreferenceService.getApplicableForProject).toHaveBeenCalledWith(user, 'p-1'); + expect(aiPreferenceService.getApplicableAcrossProjects).not.toHaveBeenCalled(); + expect( + (result.structuredContent as { preferences: Array<{ scope: string }> }).preferences.map( + (item) => item.scope, + ), + ).toEqual(['instance', 'user', 'project']); + }); + + test('answers an empty project definitely, naming the project scope', async () => { + const { aiPreferenceService, telemetry } = createMocks(empty); + const tool = createGetUserPreferencesTool(user, aiPreferenceService, telemetry); + + const result = await tool.handler({ projectId: 'p-1' }); + + expect(result.content).toEqual([{ type: 'text', text: NOTHING_SAVED_FOR_PROJECT }]); + expect(result.structuredContent).toEqual({ hasPreferences: false, preferences: [] }); + }); + + test('rejects a project the caller cannot read as an error result, not an empty success', async () => { + const { aiPreferenceService, telemetry } = createMocks( + new Error('Project with id p-9 not found'), + ); + const tool = createGetUserPreferencesTool(user, aiPreferenceService, telemetry); + + const result = await tool.handler({ projectId: 'p-9' }); + + expect(result.isError).toBe(true); + expect(result.structuredContent).toEqual({ + hasPreferences: false, + preferences: [], + error: 'Project with id p-9 not found', + }); + expect(result.content).not.toEqual([{ type: 'text', text: NOTHING_SAVED }]); + expect(result.content).not.toEqual([{ type: 'text', text: NOTHING_SAVED_FOR_PROJECT }]); + }); + }); + /** * Same decision table `ai-preference.service.test.ts` uses for `renderAiPreferences` and * `flattenAiPreferences`, applied here to the tool's own output: each group is either @@ -348,5 +406,33 @@ describe('get-user-preferences MCP tool', () => { results: { success: false, error: 'db down' }, }); }); + + test('reports the projectId a narrowed call passed', async () => { + const { aiPreferenceService, telemetry } = createMocks(empty); + const tool = createGetUserPreferencesTool(user, aiPreferenceService, telemetry); + + await tool.handler({ projectId: 'p-1' }); + + expect(telemetry.track).toHaveBeenCalledWith( + USER_CALLED_MCP_TOOL_EVENT, + expect.objectContaining({ parameters: { projectId: 'p-1' } }), + ); + }); + + test('reports a rejected projectId as a failure that still names the projectId', async () => { + const { aiPreferenceService, telemetry } = createMocks( + new Error('Project with id p-9 not found'), + ); + const tool = createGetUserPreferencesTool(user, aiPreferenceService, telemetry); + + await tool.handler({ projectId: 'p-9' }); + + expect(telemetry.track).toHaveBeenCalledWith(USER_CALLED_MCP_TOOL_EVENT, { + user_id: 'user-1', + tool_name: 'get_user_preferences', + parameters: { projectId: 'p-9' }, + results: { success: false, error: 'Project with id p-9 not found' }, + }); + }); }); }); diff --git a/packages/cli/src/modules/mcp/tools/get-user-preferences.tool.ts b/packages/cli/src/modules/mcp/tools/get-user-preferences.tool.ts index c7fdb56d937..1d159ba16fb 100644 --- a/packages/cli/src/modules/mcp/tools/get-user-preferences.tool.ts +++ b/packages/cli/src/modules/mcp/tools/get-user-preferences.tool.ts @@ -19,12 +19,24 @@ import type { ToolDefinition, UserCalledMCPToolEventPayload } from '../mcp.types const DESCRIPTION = [ 'Returns the preferences saved for this n8n instance, the caller, and their projects: node and credential choices, naming, how work is organised, and patterns to avoid.', `Call this before ${MCP_USER_PREFERENCES_TRIGGER_CLAUSE} and apply what it returns to every change you make for the remainder of the task, not only the first one. If a preference conflicts with something the user asks for directly, follow the user and say which preference you set aside.`, + 'When you work inside one project, pass its `projectId` to leave the other projects out.', ].join('\n\n'); /** A definite answer, so the assistant does not call again looking for one. */ const NOTHING_SAVED = 'No preferences are saved for this instance, for you, or for your projects.'; -const inputSchema = {} satisfies z.ZodRawShape; +const NOTHING_SAVED_FOR_PROJECT = + 'No preferences are saved for this instance, for you, or for this project.'; + +const inputSchema = { + projectId: z + .string() + .min(1) + .optional() + .describe( + 'Read one project instead of every project you can see. The instance and personal preferences are always included. Obtain the id from search_projects.', + ), +} satisfies z.ZodRawShape; const outputSchema = { hasPreferences: z @@ -91,21 +103,27 @@ export const createGetUserPreferencesTool = ( openWorldHint: false, }, }, - handler: async () => { + handler: async ({ projectId }: { projectId?: string }) => { const telemetryPayload: UserCalledMCPToolEventPayload = { user_id: user.id, tool_name: MCP_GET_USER_PREFERENCES_TOOL_NAME, - parameters: {}, + parameters: { projectId }, }; try { // The OAuth grant decides whether this client may call the tool. No RBAC check: the // service only returns rows the user may see, and their own rows need no scope. - const preferences = await aiPreferenceService.getApplicableAcrossProjects(user); + const preferences = projectId + ? await aiPreferenceService.getApplicableForProject(user, projectId) + : await aiPreferenceService.getApplicableAcrossProjects(user); // One source for "is there anything", so the flag and the list cannot disagree. const items = flattenAiPreferences(preferences); const hasPreferences = items.length > 0; - const text = hasPreferences ? renderAiPreferences(preferences) : NOTHING_SAVED; + const text = hasPreferences + ? renderAiPreferences(preferences) + : projectId + ? NOTHING_SAVED_FOR_PROJECT + : NOTHING_SAVED; // Count, scopes and the size of the rendered text, never the text itself, which is // the person's own writing. The length is what reviews the caps: CONTEXT-137 wants a diff --git a/packages/cli/src/services/__tests__/ai-preference.service.test.ts b/packages/cli/src/services/__tests__/ai-preference.service.test.ts index 800bf2781bd..ecdf7a27346 100644 --- a/packages/cli/src/services/__tests__/ai-preference.service.test.ts +++ b/packages/cli/src/services/__tests__/ai-preference.service.test.ts @@ -12,6 +12,7 @@ import { AI_PREFERENCE_MAX_PER_SCOPE } from '@n8n/api-types'; import { GLOBAL_MEMBER_ROLE, GLOBAL_OWNER_ROLE } from '@n8n/db'; import { mock } from 'vitest-mock-extended'; +import { NotFoundError } from '@/errors/response-errors/not-found.error'; import { AiPreferenceService, buildAppliedPreferencesPayload, @@ -228,6 +229,91 @@ describe('AiPreferenceService', () => { }); }); + describe('getApplicableForProject', () => { + const ownPersonal = mock({ + id: 'personal-1', + name: 'Me ', + type: 'personal', + }); + const joinedTeam = mock({ id: 'team-1', name: 'Sales', type: 'team' }); + + const relation = (projectId: string, scopes: string[]) => + ({ + projectId, + role: { scopes: scopes.map((slug) => ({ slug })) }, + }) as unknown as ProjectRelation; + const reader = (projectId: string) => relation(projectId, ['projectAiPreference:read']); + + it('rejects a project the caller may not read, instead of answering empty', async () => { + const user = mock({ id: 'user-1', role: GLOBAL_MEMBER_ROLE }); + projectRelationRepository.findAllByUser.mockResolvedValue([ + relation('team-1', ['agent:execute', 'workflow:execute-chat']), + ]); + + await expect(service.getApplicableForProject(user, 'team-1')).rejects.toThrow(NotFoundError); + expect(aiPreferenceRepository.findApplicable).not.toHaveBeenCalled(); + }); + + it('rejects a project that does not exist, even for an owner', async () => { + const owner = mock({ id: 'owner-1', role: GLOBAL_OWNER_ROLE }); + projectRepository.findOneBy.mockResolvedValue(null); + + await expect(service.getApplicableForProject(owner, 'gone')).rejects.toThrow(NotFoundError); + }); + + it("rejects another user's personal project, even for an owner", async () => { + const owner = mock({ id: 'owner-1', role: GLOBAL_OWNER_ROLE }); + const othersPersonal = mock({ + id: 'personal-2', + name: 'Them ', + type: 'personal', + }); + projectRepository.findOneBy.mockResolvedValue(othersPersonal); + projectRepository.getPersonalProjectForUser.mockResolvedValue(ownPersonal); + + await expect(service.getApplicableForProject(owner, 'personal-2')).rejects.toThrow( + NotFoundError, + ); + expect(aiPreferenceRepository.findApplicable).not.toHaveBeenCalled(); + }); + + it("serves the caller's own personal project, typed so it folds into their personal rows", async () => { + const user = mock({ id: 'user-1', role: GLOBAL_MEMBER_ROLE }); + projectRelationRepository.findAllByUser.mockResolvedValue([reader('personal-1')]); + projectRepository.findOneBy.mockResolvedValue(ownPersonal); + projectRepository.getPersonalProjectForUser.mockResolvedValue(ownPersonal); + aiPreferenceRepository.findApplicable.mockResolvedValue([ + row({ content: 'Mine', projectId: 'personal-1' }), + ]); + + const result = await service.getApplicableForProject(user, 'personal-1'); + + expect(result.projects).toEqual([ + { id: 'personal-1', name: 'Me ', type: 'personal', items: saved('Mine') }, + ]); + }); + + it('queries the one project only, for a member whose role reads it', async () => { + const user = mock({ id: 'user-1', role: GLOBAL_MEMBER_ROLE }); + projectRelationRepository.findAllByUser.mockResolvedValue([reader('team-1')]); + projectRepository.findOneBy.mockResolvedValue(joinedTeam); + aiPreferenceRepository.findApplicable.mockResolvedValue([ + row({ content: 'Sales rule', projectId: 'team-1' }), + ]); + + const result = await service.getApplicableForProject(user, 'team-1'); + + expect(projectRepository.findOneBy).toHaveBeenCalledWith({ id: 'team-1' }); + expect(aiPreferenceRepository.findApplicable).toHaveBeenCalledWith({ + userId: 'user-1', + projectIds: ['team-1'], + }); + expect(result.projects).toEqual([ + { id: 'team-1', name: 'Sales', type: 'team', items: saved('Sales rule') }, + ]); + }); + }); + describe('the scopes a write reports back', () => { const member = mock({ id: 'user-1', role: GLOBAL_MEMBER_ROLE }); const teamProject = mock({ id: 'p-1', name: 'Marketing', type: 'team' }); diff --git a/packages/cli/src/services/ai-preference.service.ts b/packages/cli/src/services/ai-preference.service.ts index bb76a8c1ea3..72b595c04d7 100644 --- a/packages/cli/src/services/ai-preference.service.ts +++ b/packages/cli/src/services/ai-preference.service.ts @@ -133,6 +133,19 @@ export class AiPreferenceService { return groupAiPreferences(rows, projects); } + async getApplicableForProject(user: User, projectId: string): Promise { + const readable = await this.projectAccess(user).has(projectId, 'read'); + const project = readable ? await this.projectRepository.findOneBy({ id: projectId }) : null; + // Global access does not include another user's personal project. + const foreignPersonal = + project?.type === 'personal' && + (await this.projectRepository.getPersonalProjectForUser(user.id))?.id !== project.id; + if (!project || foreignPersonal) { + throw new NotFoundError(`Project with id ${projectId} not found`); + } + return await this.getApplicable(user.id, [project]); + } + /** * For callers with no current project, such as the MCP server. A project counts when the * caller may read its preferences, the same rule as the REST read, so a membership that From 399da28fda1eadf693c30d2f512875df1692411d Mon Sep 17 00:00:00 2001 From: Charlie Kolb Date: Fri, 18 Sep 2026 09:07:10 +0000 Subject: [PATCH 04/67] ci: Enforce OWNERS required reviews on every base branch and fork PR (no-changelog) (#38713) Co-authored-by: Claude Fable 5.1 --- .github/WORKFLOWS.md | 42 +++- .github/scripts/github-helpers.mjs | 20 ++ .github/scripts/owners/required-reviews.mjs | 96 ++++++++- .../scripts/owners/required-reviews.test.mjs | 187 ++++++++++++++++-- .../workflows/ci-owners-required-reviews.yml | 114 ++++++----- .github/workflows/ci-pull-request-review.yml | 5 +- packages/testing/code-health/src/index.ts | 3 +- 7 files changed, 378 insertions(+), 89 deletions(-) diff --git a/.github/WORKFLOWS.md b/.github/WORKFLOWS.md index f3a3f9d8a26..2b926ba06b9 100644 --- a/.github/WORKFLOWS.md +++ b/.github/WORKFLOWS.md @@ -889,14 +889,42 @@ PR changes and review events, and reports a commit status named **Required Reviews** on the head SHA. A missing approval reports `pending` ("Waiting for approval from: …"), not `failure`, so an unreviewed PR does not show red CI; any non-success state blocks the merge equally. The -ruleset for `master` must list -that status as a required check for the block to take effect. Merge-queue runs -report success on the queue head without re-evaluating: a PR cannot enter the -queue unless the status is green on its head, and the queue does not change -approvals. +ruleset for a branch must list that status as a required check for the block +to take effect. Merge-queue runs report success on the queue head without +re-evaluating: a PR cannot enter the queue unless the status is green on its +head, and the queue does not change approvals. -The workflow reads OWNERS and its scripts from the base branch only, so a PR -cannot lift its own review requirement. +The status is evaluated for a PR into any base branch, from a same-repo head +or a fork. Both matter because the ruleset that gates a PR is not always the +one on its base branch: GitHub applies the ruleset of a stack's target branch +to every PR in the stack, so a stacked PR into a feature branch is gated by +the `master` ruleset. A required status that no run ever writes leaves the PR +blocked on "Expected". Routes that skip the evaluation are listed in +`REQUIRED_REVIEW_EXEMPTIONS` in +`required-reviews.mjs`. An entry is ` -> ` or just `` +(any head); `*` matches any run of characters. An exempt PR reports +`success` with the route in the description. Only heads in this repository +can match, so a fork branch with a matching name is still evaluated. Add a +route only when every commit it carries was already reviewed elsewhere, as +with `sync/master-to-3x -> 3.x`: its commits landed on `master` first. + +The workflow reads OWNERS, its scripts and the exemption routes from `master` +only, never from the base branch or the PR: any writable branch can be a base, +so only `master` is trusted input. A PR cannot lift its own review +requirement. A retarget re-evaluates the PR, so a verdict computed against the +old base does not carry over. + +Every path that writes the status runs in the base repository context, because +a fork-context run has no secrets and a read-only token. PR changes arrive +through `pull_request_target`, which is safe here because no step checks out +or runs PR code. Review events on a same-repo PR arrive through +`pull_request_review`. Review events on a fork PR arrive through the +`workflow_run` of `ci-pull-request-review.yml`, which runs on every submitted +or dismissed review; the owners workflow looks the PR up from that run's head +and skips same-repo heads, which the direct event already covers. A first +contribution whose runs still wait for approval gets no review-event +re-evaluation until a maintainer approves the runs; `workflow_dispatch` with +the PR number is the manual fallback. ### Transition from CODEOWNERS diff --git a/.github/scripts/github-helpers.mjs b/.github/scripts/github-helpers.mjs index bb2430deece..c579de3423d 100644 --- a/.github/scripts/github-helpers.mjs +++ b/.github/scripts/github-helpers.mjs @@ -414,6 +414,26 @@ export async function getPullRequestById(pullRequestId) { return pullRequest.data; } +/** + * Open PRs whose head is `headOwner:headBranch`, newest first. + * + * @param { string } headOwner Owner of the head repository (a fork owner or this org). + * @param { string } headBranch + * @returns { Promise } + */ +export async function listOpenPullRequestsByHead(headOwner, headBranch) { + const { octokit, owner, repo } = initGithub(); + + const pullRequests = await octokit.rest.pulls.list({ + owner, + repo, + state: 'open', + head: `${headOwner}:${headBranch}`, + }); + + return pullRequests.data; +} + /** * Returns the set of files changed in a PR, including previous filenames for renames. * diff --git a/.github/scripts/owners/required-reviews.mjs b/.github/scripts/owners/required-reviews.mjs index e0e624b292f..863978df58b 100644 --- a/.github/scripts/owners/required-reviews.mjs +++ b/.github/scripts/owners/required-reviews.mjs @@ -8,6 +8,9 @@ * runs do not reach this script: the workflow reports success on the queue * head directly, because entering the queue already required a green status. * + * Every base branch is enforced unless the PR matches a route in + * REQUIRED_REVIEW_EXEMPTIONS; an exempt PR reports success without evaluation. + * * The job itself always succeeds when the evaluation runs; the commit status * carries the verdict. The status is set to pending before the evaluation * starts, so a crash (e.g. the team membership API is unavailable) cannot @@ -20,12 +23,30 @@ import { getPrReviews, getPullRequestById, isTeamMember, + listOpenPullRequestsByHead, setCommitStatus, } from '../github-helpers.mjs'; import { parseOwnersFile, resolveRequiredTeams, teamHandleToSlug } from './owners.mjs'; export const STATUS_CONTEXT = 'Required Reviews'; -const TARGET_BRANCH = 'master'; + +/** + * PR routes that skip the evaluation. Each entry is ` -> ` or + * just `` (any head). `*` matches any run of characters, including + * `/`. Only heads in this repository qualify; a fork branch with a + * matching name does not. + * + * Add a route here only when every commit it carries was already reviewed + * elsewhere, like the master-to-3.x sync, whose commits landed on master. + */ +export const REQUIRED_REVIEW_EXEMPTIONS = ['sync/master-to-3x -> 3.x']; + +/** + * @typedef ExemptRoute + * @property { string } head Glob for the head branch. + * @property { string } base Glob for the base branch. + * @property { string } source The entry as written. + */ /** * A PR review as returned by the reviews API. @@ -36,17 +57,68 @@ const TARGET_BRANCH = 'master'; * @property { string } [submitted_at] */ +/** + * @param { string } entry ` -> ` or `` + * @returns { ExemptRoute } + */ +export function parseExemption(entry) { + const parts = entry.split('->').map((part) => part.trim()); + if (parts.length > 2 || parts.some((part) => part === '')) { + throw new Error(`Invalid exemption "${entry}": expected " -> " or ""`); + } + const [head, base] = parts.length === 2 ? parts : ['*', parts[0]]; + return { head, base, source: entry }; +} + +/** + * @param { string } ref Branch name. + * @param { string } pattern Glob where `*` matches any run of characters. + * @returns { boolean } + */ +export function matchesBranchPattern(ref, pattern) { + const escaped = pattern.split('*').map((part) => part.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')); + return new RegExp(`^${escaped.join('.*')}$`).test(ref); +} + +/** + * @param { { head: { ref: string, repo: { full_name: string } | null }, base: { ref: string, repo: { full_name: string } } } } pullRequest + * @param { string[] } [exemptions] + * @returns { ExemptRoute | undefined } The first matching route. + */ +export function findExemption(pullRequest, exemptions = REQUIRED_REVIEW_EXEMPTIONS) { + // A fork can name its branch anything, so the head must live in this repo. + if (pullRequest.head.repo?.full_name !== pullRequest.base.repo.full_name) return undefined; + + return exemptions + .map(parseExemption) + .find( + (route) => + matchesBranchPattern(pullRequest.head.ref, route.head) && + matchesBranchPattern(pullRequest.base.ref, route.base), + ); +} + /** * Resolve the PR number from the triggering event. * + * A workflow_run event carries no PR: the PR is looked up from the + * triggering run's head. Only an open PR whose head SHA is the run's head + * SHA counts; several PRs can share a head branch. + * * @param { string } eventName * @param { any } event Parsed GITHUB_EVENT_PATH payload. * @param { string | undefined } pullRequestNumberEnv PULL_REQUEST_NUMBER (workflow_dispatch input). - * @returns { number } + * @returns { Promise } undefined when the event has no open PR to evaluate. */ -export function resolvePullRequestNumber(eventName, event, pullRequestNumberEnv) { +export async function resolvePullRequestNumber(eventName, event, pullRequestNumberEnv) { if (event?.pull_request?.number) return event.pull_request.number; + if (eventName === 'workflow_run' && event?.workflow_run) { + const { head_sha: sha, head_branch: branch, head_repository: headRepository } = event.workflow_run; + const candidates = await listOpenPullRequestsByHead(headRepository.owner.login, branch); + return candidates.find((pullRequest) => pullRequest.head.sha === sha)?.number; + } + const parsed = parseInt(pullRequestNumberEnv ?? ''); if (Number.isNaN(parsed)) { throw new Error(`Cannot resolve a PR number for event "${eventName}"`); @@ -160,18 +232,16 @@ export async function run() { const eventName = process.env.GITHUB_EVENT_NAME ?? 'workflow_dispatch'; const event = getEventFromGithubEventPath(); - const pullRequestNumber = resolvePullRequestNumber( + const pullRequestNumber = await resolvePullRequestNumber( eventName, event, process.env.PULL_REQUEST_NUMBER, ); - const pullRequest = await getPullRequestById(pullRequestNumber); - - if (pullRequest.base.ref !== TARGET_BRANCH) { - console.log(`PR #${pullRequestNumber} targets "${pullRequest.base.ref}", not "${TARGET_BRANCH}"; skipping.`); + if (pullRequestNumber === undefined) { + console.log(`No open PR for the ${eventName} head; nothing to evaluate.`); return; } - + const pullRequest = await getPullRequestById(pullRequestNumber); const statusSha = pullRequest.head.sha; const targetUrl = statusTargetUrl(); @@ -187,7 +257,13 @@ export async function run() { /** @type { { state: 'success' | 'pending', description: string } } */ let status; try { - status = await evaluateRequiredReviews(pullRequestNumber); + const exemption = findExemption(pullRequest); + if (exemption) { + console.log(`PR #${pullRequestNumber} matches exempt route "${exemption.source}"; skipping the evaluation.`); + status = { state: 'success', description: `Exempt route: ${exemption.source}` }; + } else { + status = await evaluateRequiredReviews(pullRequestNumber); + } } catch (evaluationError) { // Best effort: nicer than a stuck pending status. The pending status // already blocks the merge if this write fails too. diff --git a/.github/scripts/owners/required-reviews.test.mjs b/.github/scripts/owners/required-reviews.test.mjs index 1b5bd7885ee..3b20341cb6c 100644 --- a/.github/scripts/owners/required-reviews.test.mjs +++ b/.github/scripts/owners/required-reviews.test.mjs @@ -19,6 +19,8 @@ let getPrReviewsImpl = async () => []; let isTeamMemberImpl = async () => false; /** @type {(sha: string, status: any) => Promise} */ let setCommitStatusImpl = async () => {}; +/** @type {(headOwner: string, headBranch: string) => Promise} */ +let listOpenPullRequestsByHeadImpl = async () => []; mock.module('../github-helpers.mjs', { namedExports: { @@ -28,6 +30,8 @@ mock.module('../github-helpers.mjs', { getPrReviews: (n) => getPrReviewsImpl(n), isTeamMember: (slug, username) => isTeamMemberImpl(slug, username), setCommitStatus: (sha, status) => setCommitStatusImpl(sha, status), + listOpenPullRequestsByHead: (headOwner, headBranch) => + listOpenPullRequestsByHeadImpl(headOwner, headBranch), }, }); @@ -46,28 +50,150 @@ mock.module('./owners.mjs', { }); const { + REQUIRED_REVIEW_EXEMPTIONS, STATUS_CONTEXT, buildStatus, collectApprovers, + findExemption, latestReviewStates, + matchesBranchPattern, + parseExemption, resolvePullRequestNumber, run, } = await import('./required-reviews.mjs'); +const REPO = 'n8n-io/n8n'; + +/** @param {string} head @param {string} base @param {string} [headRepo] */ +function pullRequestFor(head, base, headRepo = REPO) { + return { + head: { ref: head, sha: 'head-sha', repo: { full_name: headRepo } }, + base: { ref: base, repo: { full_name: REPO } }, + }; +} + +describe('parseExemption', () => { + it('splits a " -> " route', () => { + assert.deepEqual(parseExemption('sync/master-to-3x -> 3.x'), { + head: 'sync/master-to-3x', + base: '3.x', + source: 'sync/master-to-3x -> 3.x', + }); + }); + + it('treats a bare "" as any head', () => { + assert.deepEqual(parseExemption('release/*'), { head: '*', base: 'release/*', source: 'release/*' }); + }); + + it('rejects malformed routes', () => { + assert.throws(() => parseExemption('a -> b -> c'), /Invalid exemption/); + assert.throws(() => parseExemption(' -> 3.x'), /Invalid exemption/); + assert.throws(() => parseExemption('3.x -> '), /Invalid exemption/); + }); + + it('accepts every configured exemption', () => { + for (const entry of REQUIRED_REVIEW_EXEMPTIONS) parseExemption(entry); + }); +}); + +describe('matchesBranchPattern', () => { + it('matches whole branch names', () => { + assert.equal(matchesBranchPattern('3.x', '3.x'), true); + assert.equal(matchesBranchPattern('3.xy', '3.x'), false); + assert.equal(matchesBranchPattern('v3.x', '3.x'), false); + }); + + it('treats "." as a literal', () => { + assert.equal(matchesBranchPattern('3ax', '3.x'), false); + }); + + it('expands "*" across path separators', () => { + assert.equal(matchesBranchPattern('release/2.39.5', 'release/*'), true); + assert.equal(matchesBranchPattern('release/2.39.5/hotfix', 'release/*'), true); + assert.equal(matchesBranchPattern('anything', '*'), true); + assert.equal(matchesBranchPattern('release-candidate/2.40.0', 'release/*'), false); + }); +}); + +describe('findExemption', () => { + const exemptions = ['sync/master-to-3x -> 3.x', 'release/*']; + + it('finds the route for a matching head and base', () => { + const route = findExemption(pullRequestFor('sync/master-to-3x', '3.x'), exemptions); + assert.equal(route?.source, 'sync/master-to-3x -> 3.x'); + }); + + it('returns nothing when only the base matches', () => { + assert.equal(findExemption(pullRequestFor('feature/x', '3.x'), exemptions), undefined); + }); + + it('returns nothing when only the head matches', () => { + assert.equal(findExemption(pullRequestFor('sync/master-to-3x', 'master'), exemptions), undefined); + }); + + it('matches a bare base route for any head', () => { + const route = findExemption(pullRequestFor('feature/x', 'release/2.39.5'), exemptions); + assert.equal(route?.source, 'release/*'); + }); + + it('ignores heads from another repository', () => { + const fromFork = pullRequestFor('sync/master-to-3x', '3.x', 'someone/n8n'); + assert.equal(findExemption(fromFork, exemptions), undefined); + }); + + it('ignores heads whose repository is gone', () => { + const pullRequest = pullRequestFor('sync/master-to-3x', '3.x'); + pullRequest.head.repo = null; + assert.equal(findExemption(pullRequest, exemptions), undefined); + }); +}); + describe('resolvePullRequestNumber', () => { - it('reads the PR number from pull_request payloads', () => { + /** A workflow_run payload for a run on `owner/branch` at `sha`. */ + function workflowRunEvent(owner, branch, sha) { + return { workflow_run: { head_sha: sha, head_branch: branch, head_repository: { owner: { login: owner } } } }; + } + + beforeEach(() => { + listOpenPullRequestsByHeadImpl = async () => []; + }); + + it('reads the PR number from pull_request payloads', async () => { assert.equal( - resolvePullRequestNumber('pull_request', { pull_request: { number: 42 } }, undefined), + await resolvePullRequestNumber('pull_request_target', { pull_request: { number: 42 } }, undefined), 42, ); }); - it('falls back to the PULL_REQUEST_NUMBER env value for workflow_dispatch', () => { - assert.equal(resolvePullRequestNumber('workflow_dispatch', {}, '7'), 7); + it('falls back to the PULL_REQUEST_NUMBER env value for workflow_dispatch', async () => { + assert.equal(await resolvePullRequestNumber('workflow_dispatch', {}, '7'), 7); }); - it('throws when no source yields a PR number', () => { - assert.throws(() => resolvePullRequestNumber('workflow_dispatch', {}, undefined), /Cannot resolve/); + it('rejects when no source yields a PR number', async () => { + await assert.rejects(resolvePullRequestNumber('workflow_dispatch', {}, undefined), /Cannot resolve/); + }); + + it('looks up the open PR for a workflow_run head', async () => { + const listOpenPullRequestsByHead = mock.fn(async () => [ + { number: 11, head: { sha: 'old-sha' } }, + { number: 12, head: { sha: 'run-sha' } }, + ]); + listOpenPullRequestsByHeadImpl = listOpenPullRequestsByHead; + + assert.equal( + await resolvePullRequestNumber('workflow_run', workflowRunEvent('forker', 'fix/x', 'run-sha'), undefined), + 12, + ); + assert.deepEqual(listOpenPullRequestsByHead.mock.calls[0].arguments, ['forker', 'fix/x']); + }); + + it('returns undefined when no open PR has the workflow_run head SHA', async () => { + listOpenPullRequestsByHeadImpl = async () => [{ number: 11, head: { sha: 'old-sha' } }]; + + assert.equal( + await resolvePullRequestNumber('workflow_run', workflowRunEvent('forker', 'fix/x', 'run-sha'), undefined), + undefined, + ); }); }); @@ -152,8 +278,7 @@ describe('run', () => { eventImpl = () => ({ pull_request: { number: 42 } }); getPullRequestByIdImpl = async () => ({ - base: { ref: 'master' }, - head: { sha: 'head-sha' }, + ...pullRequestFor('feature/x', 'master'), user: { login: 'author' }, }); getChangedFilesImpl = async () => new Set(['a.ts']); @@ -267,15 +392,53 @@ describe('run', () => { assert.equal(status.state, 'pending'); }); - it('skips PRs that do not target master without setting a status', async () => { - getPullRequestByIdImpl = async () => ({ - base: { ref: 'release-candidate/2.9.x' }, - head: { sha: 'head-sha' }, + it('writes no status for a workflow_run head without an open PR', async () => { + process.env.GITHUB_EVENT_NAME = 'workflow_run'; + eventImpl = () => ({ + workflow_run: { head_sha: 'gone-sha', head_branch: 'fix/x', head_repository: { owner: { login: 'forker' } } }, }); + listOpenPullRequestsByHeadImpl = async () => []; await run(); assert.equal(setCommitStatus.mock.calls.length, 0); }); + it('evaluates PRs into branches other than master', async () => { + getPullRequestByIdImpl = async () => pullRequestFor('feature/x', 'release-candidate/2.9.x'); + resolveRequiredTeamsImpl = () => new Map([['@n8n-io/qa-dx', ['a.ts']]]); + + await run(); + + const [, status] = setCommitStatus.mock.calls.at(-1).arguments; + assert.equal(status.state, 'pending'); + assert.match(status.description, /Waiting for approval from: qa-dx/); + }); + + it('reports success without evaluating a PR on an exempt route', async () => { + getPullRequestByIdImpl = async () => pullRequestFor('sync/master-to-3x', '3.x'); + const getChangedFiles = mock.fn(async () => new Set(['a.ts'])); + getChangedFilesImpl = getChangedFiles; + resolveRequiredTeamsImpl = () => new Map([['@n8n-io/qa-dx', ['a.ts']]]); + + await run(); + + assert.equal(getChangedFiles.mock.calls.length, 0); + assert.equal(setCommitStatus.mock.calls.length, 2); + assert.equal(setCommitStatus.mock.calls[0].arguments[1].state, 'pending'); + const [, status] = setCommitStatus.mock.calls[1].arguments; + assert.equal(status.state, 'success'); + assert.equal(status.description, 'Exempt route: sync/master-to-3x -> 3.x'); + }); + + it('does not exempt a fork branch that is named like an exempt route', async () => { + getPullRequestByIdImpl = async () => pullRequestFor('sync/master-to-3x', '3.x', 'someone/n8n'); + resolveRequiredTeamsImpl = () => new Map([['@n8n-io/qa-dx', ['a.ts']]]); + + await run(); + + const [, status] = setCommitStatus.mock.calls.at(-1).arguments; + assert.equal(status.state, 'pending'); + }); + }); diff --git a/.github/workflows/ci-owners-required-reviews.yml b/.github/workflows/ci-owners-required-reviews.yml index 828bef78787..17112e513d8 100644 --- a/.github/workflows/ci-owners-required-reviews.yml +++ b/.github/workflows/ci-owners-required-reviews.yml @@ -8,25 +8,44 @@ run-name: "${{ github.event_name == 'workflow_dispatch' && format('CI: Owners Re # Replaces the GitHub-native CODEOWNERS enforcement, which could not express # team ownership metadata beyond "request a review". # +# Every PR is evaluated, whatever its base branch and whether or not its head +# is a fork. Two PR classes make this necessary: GitHub applies the ruleset of +# a stack's target branch to every PR in the stack, so a PR into a feature +# branch can be gated by the master ruleset; and a fork PR cannot write the +# status from a fork-context run. Every path that reports the status runs +# in the base repository context (secrets, write token). The routes that +# skip the evaluation (for example the master-to-3.x sync branch) live in +# REQUIRED_REVIEW_EXEMPTIONS in required-reviews.mjs; an exempt PR gets a +# success status. +# # Triggers -# - pull_request (opened/synchronize/reopened/ready_for_review): -# re-evaluates whenever the changeset can change. Skipped for fork PRs -# (no secrets): their status stays "expected" — fail closed — until the -# first review event reports it. +# - pull_request_target (opened/synchronize/reopened/ready_for_review, and +# edited when the base branch changes): re-evaluates whenever the +# changeset can change, for same-repo and fork heads alike. A retargeted +# PR keeps its head SHA, so a verdict computed against the old base must +# not survive the retarget. # - pull_request_review (submitted/dismissed): re-evaluates when approvals -# change. A privileged event (base repo context, secrets), so it covers -# fork PRs too — and approvals can only change through review events. +# change on a same-repo PR. A fork PR's review event runs without secrets +# and with a read-only token, so this path skips fork heads. # Comment-only reviews are skipped: they cannot change approvals, and # review bots submit one after every push. +# - workflow_run of "CI: Pull Request Review" (completed): the fork +# counterpart of pull_request_review. That workflow runs on every review +# event; its completion arrives here in the base repository context. The +# PR is looked up from the triggering run's head. Same-repo heads skip +# this path, which pull_request_review already covers. # - merge_group: reports success on the queue head without re-evaluating. # A PR cannot enter the queue unless this status is green on its head, # and the queue does not change approvals. -# - workflow_dispatch: manual re-check (e.g. after a team membership change, -# or a fork PR push after an approval). +# - workflow_dispatch: manual re-check (e.g. after a team membership +# change). # -# The checkout below always pins the base branch, never the PR merge ref: the -# scripts and the OWNERS file are trusted input, so a PR cannot lift its own -# review requirement or reach the status-writing token. +# The checkout below always pins master, never the base branch or the PR +# merge ref: the scripts, the OWNERS file and the exemption routes are trusted +# input, so a PR cannot lift its own review requirement or reach the +# status-writing token. Any branch can be a base, and anyone with push access +# can edit a branch, so only master is trusted. This is also what makes +# pull_request_target safe here: no step checks out or runs PR code. # # Output # - A commit status named "Required Reviews" on the PR head SHA (or @@ -38,12 +57,13 @@ run-name: "${{ github.event_name == 'workflow_dispatch' && format('CI: Owners Re # stale verdict in effect. The job itself only fails on errors. on: - pull_request: - types: [opened, synchronize, reopened, ready_for_review] - branches: - - master + pull_request_target: + types: [opened, synchronize, reopened, ready_for_review, edited] pull_request_review: types: [submitted, dismissed] + workflow_run: + workflows: ['CI: Pull Request Review'] + types: [completed] merge_group: workflow_dispatch: inputs: @@ -90,24 +110,29 @@ jobs: required-reviews: name: Check required reviews - # Fork PRs are skipped on pull_request (no secrets for the app token); - # pull_request_review covers them. pull_request_review has no `branches` - # filter; the script also skips non-master bases, the guard here keeps - # the early pending status off non-master heads. + # Of the edited events only a base change matters; title and body edits + # do not change the verdict. Review events split by head: same-repo + # heads come in through pull_request_review, fork heads through the + # workflow_run of the review workflow. if: >- github.event_name != 'merge_group' && - (github.event_name != 'pull_request' || - github.event.pull_request.head.repo.full_name == github.repository) && + (github.event_name != 'pull_request_target' || + github.event.action != 'edited' || github.event.changes.base) && (github.event_name != 'pull_request_review' || - (github.event.pull_request.base.ref == 'master' && - github.event.review.state != 'commented')) + (github.event.pull_request.head.repo.full_name == github.repository && + github.event.review.state != 'commented')) && + (github.event_name != 'workflow_run' || + (github.event.workflow_run.event == 'pull_request_review' && + github.event.workflow_run.head_repository.full_name != github.repository)) runs-on: ubuntu-latest timeout-minutes: 5 # Job-level, not workflow-level: a run whose job is skipped must not - # cancel an in-flight evaluation, or the status stays pending. + # cancel an in-flight evaluation, or the status stays pending. Keyed by + # head SHA, so the paths above share one group per PR head. concurrency: group: >- - owners-required-reviews-${{ github.event.pull_request.number + owners-required-reviews-${{ github.event.pull_request.head.sha + || github.event.workflow_run.head_sha || inputs.pr_number || github.ref }} cancel-in-progress: true @@ -135,7 +160,7 @@ jobs: if: github.event_name != 'workflow_dispatch' env: GH_TOKEN: ${{ steps.app-token.outputs.token }} - SHA: ${{ github.event.pull_request.head.sha }} + SHA: ${{ github.event.pull_request.head.sha || github.event.workflow_run.head_sha }} RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} run: | gh api "repos/${GITHUB_REPOSITORY}/statuses/${SHA}" \ @@ -144,44 +169,17 @@ jobs: -f description='Evaluating required reviews' \ -f "target_url=${RUN_URL}" - # Always check out the base branch, never the PR merge ref: the OWNERS - # file and the scripts must be trusted input. The 'master' fallback - # covers workflow_dispatch, so a dispatch from another branch cannot - # run that branch's policy. The script reads only `.github/` and the - # root files, which cone mode always includes. + # Always check out master, never the base branch or the PR merge ref: + # the OWNERS file, the scripts and the exemption routes must be trusted + # input, and any writable branch can be a base. The script reads only + # `.github/` and the root files, which cone mode always includes. - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: - ref: ${{ github.event.pull_request.base.ref || 'master' }} + ref: master persist-credentials: false sparse-checkout: .github - # Bootstrap: the checkout is the trusted base branch. Until the action - # exists there, there is nothing to run. Report success so the PR that - # introduces it can merge; the check is enforced again from the next PR. - - name: Check for the enforcement action on the base branch - id: bootstrap - shell: bash - run: | - if [ -f .github/actions/run-workflow-script/action.yml ]; then - echo "ready=true" >> "$GITHUB_OUTPUT" - else - echo "ready=false" >> "$GITHUB_OUTPUT" - echo "No enforcement action on the base branch; nothing to enforce." - fi - - - name: Report the bootstrap state - if: steps.bootstrap.outputs.ready != 'true' && github.event.pull_request.head.sha != '' - env: - GH_TOKEN: ${{ steps.app-token.outputs.token }} - SHA: ${{ github.event.pull_request.head.sha }} - run: | - gh api "repos/${GITHUB_REPOSITORY}/statuses/${SHA}" \ - -f state=success \ - -f context='Required Reviews' \ - -f description='Owners enforcement is not on the base branch yet' - - name: Evaluate required reviews - if: steps.bootstrap.outputs.ready == 'true' uses: ./.github/actions/run-workflow-script env: PULL_REQUEST_NUMBER: ${{ inputs.pr_number || '' }} diff --git a/.github/workflows/ci-pull-request-review.yml b/.github/workflows/ci-pull-request-review.yml index 8692e6663d7..2c9ca0ed9d8 100644 --- a/.github/workflows/ci-pull-request-review.yml +++ b/.github/workflows/ci-pull-request-review.yml @@ -1,8 +1,11 @@ name: 'CI: Pull Request Review' +# "CI: Owners Required Reviews" listens to this workflow's completion +# (workflow_run) to re-evaluate fork PRs: a fork's review event runs without +# secrets, a workflow_run does not. Keep the trigger on every review event. on: pull_request_review: - types: [submitted] + types: [submitted, dismissed] concurrency: group: ci-review-${{ github.event.pull_request.number }} diff --git a/packages/testing/code-health/src/index.ts b/packages/testing/code-health/src/index.ts index 4412366e880..2b52fa20bd8 100644 --- a/packages/testing/code-health/src/index.ts +++ b/packages/testing/code-health/src/index.ts @@ -35,7 +35,8 @@ const defaultRuleSettings: RuleSettingsMap = { 'workflow-pr-target-safety': { enabled: true, severity: 'error', - options: { allowedWorkflows: ['ci-cla-check.yml'] }, + // ci-owners-required-reviews.yml checks out master only, never PR code. + options: { allowedWorkflows: ['ci-cla-check.yml', 'ci-owners-required-reviews.yml'] }, }, 'migration-timestamp': { enabled: true, From ec541453049fc9ff496101f45f6b7be71e1f4966 Mon Sep 17 00:00:00 2001 From: James Martin <393605+ymhr@users.noreply.github.com> Date: Fri, 18 Sep 2026 09:10:55 +0000 Subject: [PATCH 05/67] fix(core): Apply the same mandatory roles when creating role from API and frontend (#38860) Co-authored-by: Savelii --- .../__tests__/custom-role-scopes.test.ts | 68 +++++++++- .../src/roles/custom-role-scopes.ee.ts | 42 +++++- .../__tests__/role.controller.test.ts | 1 + .../cli/src/controllers/role.controller.ts | 1 + .../__tests__/telemetry-event-relay.test.ts | 2 + .../cli/src/events/maps/relay.event-map.ts | 2 + .../events/relays/telemetry.event-relay.ts | 8 +- .../__tests__/roles.public.controller.test.ts | 52 +++++++ .../v1/controllers/roles.public.controller.ts | 1 + .../role.service.customRoles.test.ts | 128 +++++++++++++++++- packages/cli/src/services/role.service.ts | 17 ++- .../test/integration/public-api/roles.test.ts | 54 ++++++-- .../integration/services/role.service.test.ts | 54 +++++++- .../roles/instance/InstanceRoleView.test.ts | 2 +- .../roles/instance/instanceRoleScopes.ts | 57 +++----- 15 files changed, 419 insertions(+), 70 deletions(-) create mode 100644 packages/cli/src/public-api/v1/controllers/__tests__/roles.public.controller.test.ts diff --git a/packages/@n8n/permissions/src/roles/__tests__/custom-role-scopes.test.ts b/packages/@n8n/permissions/src/roles/__tests__/custom-role-scopes.test.ts index 5a130d3a742..54819cd4fae 100644 --- a/packages/@n8n/permissions/src/roles/__tests__/custom-role-scopes.test.ts +++ b/packages/@n8n/permissions/src/roles/__tests__/custom-role-scopes.test.ts @@ -2,7 +2,10 @@ import { CUSTOM_ROLE_SCOPE_WHITELIST, GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS, GLOBAL_CUSTOM_ROLE_SCOPES, + isMandatoryInstanceOption, + MANDATORY_INSTANCE_SCOPES, PROJECT_CUSTOM_ROLE_SCOPES, + withMandatoryInstanceScopes, } from '@/roles/custom-role-scopes.ee'; import { GLOBAL_MEMBER_SCOPES } from '@/roles/scopes/global-scopes.ee'; import { ALL_SCOPES } from '@/scope-information'; @@ -150,8 +153,8 @@ describe('custom role scope whitelists', () => { it('keeps "Tags: View" within GLOBAL_MEMBER_SCOPES', () => { // "Tags: View" is granted to every instance role by default (see - // instanceRoleScopes.ts), so it must never exceed what the built-in Member - // role already has. + // MANDATORY_INSTANCE_OPTIONS), so it must never exceed what the built-in + // Member role already has. for (const scope of GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS.tag.View) { expect(GLOBAL_MEMBER_SCOPES).toContain(scope); } @@ -159,8 +162,8 @@ describe('custom role scope whitelists', () => { it('exposes "Users: View" as exactly user:list, matching GLOBAL_MEMBER_SCOPES', () => { // "Users: View" is granted to every instance role by default (see - // instanceRoleScopes.ts). It must never exceed what the built-in Member - // role already has, or a custom role mirroring Member ends up more + // MANDATORY_INSTANCE_OPTIONS). It must never exceed what the built-in + // Member role already has, or a custom role mirroring Member ends up more // privileged than Member itself expect(GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS.user.View).toEqual(['user:list']); for (const scope of GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS.user.View) { @@ -168,3 +171,60 @@ describe('custom role scope whitelists', () => { } }); }); + +describe('mandatory instance options', () => { + it('resolves to the user and tag view scopes', () => { + expect(MANDATORY_INSTANCE_SCOPES).toEqual(['user:list', 'tag:read', 'tag:list']); + }); + + it('stays inside the global custom-role whitelist', () => { + // resolveScopes unions these in before the whitelist check, so a mandatory + // scope outside the whitelist would make every global-role write fail. + for (const scope of MANDATORY_INSTANCE_SCOPES) { + expect(GLOBAL_CUSTOM_ROLE_SCOPES.has(scope)).toBe(true); + } + }); + + it('stays within GLOBAL_MEMBER_SCOPES', () => { + // Added to every custom instance role, so they must never exceed what the + // built-in Member role already grants. + for (const scope of MANDATORY_INSTANCE_SCOPES) { + expect(GLOBAL_MEMBER_SCOPES).toContain(scope); + } + }); + + it('identifies mandatory options and only those', () => { + expect(isMandatoryInstanceOption('user', 'View')).toBe(true); + expect(isMandatoryInstanceOption('tag', 'View')).toBe(true); + expect(isMandatoryInstanceOption('user', 'Manage')).toBe(false); + expect(isMandatoryInstanceOption('insights', 'View')).toBe(false); + expect(isMandatoryInstanceOption('nonsense', 'View')).toBe(false); + }); + + it('withMandatoryInstanceScopes adds the mandatory scopes to an empty list', () => { + expect(withMandatoryInstanceScopes([])).toEqual(['user:list', 'tag:read', 'tag:list']); + }); + + it('withMandatoryInstanceScopes does not duplicate scopes already present', () => { + const result = withMandatoryInstanceScopes(['user:list', 'tag:read']); + expect(result.filter((scope) => scope === 'user:list')).toHaveLength(1); + expect(result.filter((scope) => scope === 'tag:read')).toHaveLength(1); + expect(new Set(result).size).toBe(result.length); + }); + + it('withMandatoryInstanceScopes dedups a repeated input slug', () => { + expect(withMandatoryInstanceScopes(['insights:read', 'insights:read'])).toEqual([ + 'insights:read', + 'user:list', + 'tag:read', + 'tag:list', + ]); + }); + + it('withMandatoryInstanceScopes preserves unrelated scopes untouched', () => { + const result = withMandatoryInstanceScopes(['insights:read', 'insights:list']); + expect(result).toContain('insights:read'); + expect(result).toContain('insights:list'); + expect(result).toEqual(expect.arrayContaining([...MANDATORY_INSTANCE_SCOPES])); + }); +}); diff --git a/packages/@n8n/permissions/src/roles/custom-role-scopes.ee.ts b/packages/@n8n/permissions/src/roles/custom-role-scopes.ee.ts index 17f36143d7a..37bcb963200 100644 --- a/packages/@n8n/permissions/src/roles/custom-role-scopes.ee.ts +++ b/packages/@n8n/permissions/src/roles/custom-role-scopes.ee.ts @@ -162,8 +162,8 @@ export const GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS = { // Tags on a workflow you can read come embedded in the workflow response, and // applying one rides on workflow:update, not a tag scope. read/list gate only // the tag *picker* — listing every existing tag to choose from — which every - // role gets (see MANDATORY_INSTANCE_OPTIONS in instanceRoleScopes.ts). Manage - // keeps read/list so it stays a strict superset of View, matching `user`. + // role gets (see MANDATORY_INSTANCE_OPTIONS below). Manage keeps read/list so + // it stays a strict superset of View, matching `user`. View: ['tag:read', 'tag:list'], Manage: ['tag:read', 'tag:list', 'tag:create', 'tag:update', 'tag:delete'], }, @@ -196,6 +196,44 @@ export const GLOBAL_CUSTOM_ROLE_SCOPES: ReadonlySet = new Set( ), ); +/** Correlates each resource with its own option keys, so a typo fails the typecheck. */ +type MandatoryInstanceOption = { + [R in keyof typeof GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS]: { + resource: R; + option: keyof (typeof GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS)[R]; + }; +}[keyof typeof GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS]; + +/** + * Options every instance role carries regardless of what is saved — the default + * Member role already grants them, so they are baseline behaviour rather than + * something a custom role opts into. The editor renders them checked and + * disabled; `resolveScopes` unions them into every global-role write. + */ +export const MANDATORY_INSTANCE_OPTIONS = [ + { resource: 'user', option: 'View' }, + { resource: 'tag', option: 'View' }, +] as const satisfies readonly MandatoryInstanceOption[]; + +export function isMandatoryInstanceOption(resource: string, option: string): boolean { + return MANDATORY_INSTANCE_OPTIONS.some((m) => m.resource === resource && m.option === option); +} + +/** Derived by indexing the groups table, so a typo contributes nothing silently. */ +export const MANDATORY_INSTANCE_SCOPES: readonly Scope[] = [ + ...new Set( + MANDATORY_INSTANCE_OPTIONS.flatMap(({ resource, option }) => { + const optionMap: Record = GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS[resource]; + return optionMap[option]; + }), + ), +]; + +/** Unions in the mandatory scopes (see `MANDATORY_INSTANCE_OPTIONS`) on top of an already-filtered scope list. */ +export function withMandatoryInstanceScopes(scopes: readonly string[]): string[] { + return [...new Set([...scopes, ...MANDATORY_INSTANCE_SCOPES])]; +} + /** Precise union of the scopes the operations map grants — keeps each resource * correlated with its own operations (no resource×operation cross-product). */ type ProjectCustomRoleScope = { diff --git a/packages/cli/src/controllers/__tests__/role.controller.test.ts b/packages/cli/src/controllers/__tests__/role.controller.test.ts index e238193e324..86276712a01 100644 --- a/packages/cli/src/controllers/__tests__/role.controller.test.ts +++ b/packages/cli/src/controllers/__tests__/role.controller.test.ts @@ -40,6 +40,7 @@ describe('RoleController', () => { userId: '123', roleSlug: 'custom-editor', scopes: ['workflow:read', 'workflow:update'], + source: 'ui', }); }); }); diff --git a/packages/cli/src/controllers/role.controller.ts b/packages/cli/src/controllers/role.controller.ts index cfdf46f5c6d..79b5f373baa 100644 --- a/packages/cli/src/controllers/role.controller.ts +++ b/packages/cli/src/controllers/role.controller.ts @@ -200,6 +200,7 @@ export class RoleController { userId: req.user.id, roleSlug: result.slug, scopes: result.scopes, + source: 'ui', }); return result; } diff --git a/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts b/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts index 10634eb3874..d0e16b277b9 100644 --- a/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts +++ b/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts @@ -1302,6 +1302,7 @@ describe('TelemetryEventRelay', () => { userId: 'user123', roleSlug: 'project:my-role-abc123', scopes: ['workflow:create', 'workflow:read', 'credential:read'], + source: 'ui', }; eventService.emit('custom-role-created', event); @@ -1310,6 +1311,7 @@ describe('TelemetryEventRelay', () => { user_id: 'user123', role_slug: 'project:my-role-abc123', scopes: ['workflow:create', 'workflow:read', 'credential:read'], + source: 'ui', }); }); diff --git a/packages/cli/src/events/maps/relay.event-map.ts b/packages/cli/src/events/maps/relay.event-map.ts index 55f318cfa12..fecef9a2416 100644 --- a/packages/cli/src/events/maps/relay.event-map.ts +++ b/packages/cli/src/events/maps/relay.event-map.ts @@ -1204,6 +1204,8 @@ export type RelayEventMap = { userId: string; roleSlug: string; scopes: string[]; + /** Which surface created the role. Both reach the same service, so the caller names its own. */ + source: 'ui' | 'public-api'; }; 'custom-role-updated': { diff --git a/packages/cli/src/events/relays/telemetry.event-relay.ts b/packages/cli/src/events/relays/telemetry.event-relay.ts index 7bcfb3ed8cf..48236b961a6 100644 --- a/packages/cli/src/events/relays/telemetry.event-relay.ts +++ b/packages/cli/src/events/relays/telemetry.event-relay.ts @@ -2435,11 +2435,17 @@ export class TelemetryEventRelay extends EventRelay { // #region Custom Roles - private customRoleCreated({ userId, roleSlug, scopes }: RelayEventMap['custom-role-created']) { + private customRoleCreated({ + userId, + roleSlug, + scopes, + source, + }: RelayEventMap['custom-role-created']) { this.telemetry.track('User created custom role', { user_id: userId, role_slug: roleSlug, scopes, + source, }); } diff --git a/packages/cli/src/public-api/v1/controllers/__tests__/roles.public.controller.test.ts b/packages/cli/src/public-api/v1/controllers/__tests__/roles.public.controller.test.ts new file mode 100644 index 00000000000..049a74c9f85 --- /dev/null +++ b/packages/cli/src/public-api/v1/controllers/__tests__/roles.public.controller.test.ts @@ -0,0 +1,52 @@ +import type { CreateRoleDto } from '@n8n/api-types'; +import type { AuthenticatedRequest } from '@n8n/db'; +import type { Role } from '@n8n/permissions'; +import { mock } from 'vitest-mock-extended'; + +import type { EventService } from '@/events/event.service'; +import type { RoleService } from '@/services/role.service'; + +import { RolesPublicController } from '../roles.public.controller'; + +describe('RolesPublicController', () => { + const roleService = mock(); + const eventService = mock(); + const controller = new RolesPublicController(roleService, eventService); + + beforeEach(() => { + vi.clearAllMocks(); + }); + + // The public route reaches the same service as the role editor, so `source` is + // the only thing telling the two surfaces apart downstream. + it("createRole emits custom-role-created with source 'public-api'", async () => { + roleService.createCustomRole.mockResolvedValue({ + slug: 'global:auditor-abc123', + displayName: 'Auditor', + description: null, + systemRole: false, + roleType: 'global', + scopes: ['insights:read'], + createdAt: new Date(), + updatedAt: new Date(), + } as Role); + + const request = mock({ + user: { id: '123', role: { scopes: [{ slug: 'role:manage' }] } }, + tokenGrant: { apiKeyScopes: ['role:manage'] }, + }); + + await controller.createRole(request, mock(), { + displayName: 'Auditor', + roleType: 'global', + scopes: ['insights:read'], + } as CreateRoleDto); + + expect(eventService.emit).toHaveBeenCalledWith('custom-role-created', { + userId: '123', + roleSlug: 'global:auditor-abc123', + scopes: ['insights:read'], + source: 'public-api', + }); + }); +}); diff --git a/packages/cli/src/public-api/v1/controllers/roles.public.controller.ts b/packages/cli/src/public-api/v1/controllers/roles.public.controller.ts index 162e4807f99..3963b98c727 100644 --- a/packages/cli/src/public-api/v1/controllers/roles.public.controller.ts +++ b/packages/cli/src/public-api/v1/controllers/roles.public.controller.ts @@ -146,6 +146,7 @@ export class RolesPublicController { userId: req.user.id, roleSlug: role.slug, scopes: role.scopes, + source: 'public-api', }); return toRolePublicDto({ ...role, roleType: createRole.roleType }); diff --git a/packages/cli/src/services/__tests__/role.service.customRoles.test.ts b/packages/cli/src/services/__tests__/role.service.customRoles.test.ts index 98b91f55748..4d071b2e275 100644 --- a/packages/cli/src/services/__tests__/role.service.customRoles.test.ts +++ b/packages/cli/src/services/__tests__/role.service.customRoles.test.ts @@ -4,6 +4,7 @@ import { Logger } from '@n8n/backend-common'; import { mockInstance } from '@n8n/backend-test-utils'; import type { Role, Scope as DBScope } from '@n8n/db'; import { RoleRepository, ScopeRepository } from '@n8n/db'; +import { MANDATORY_INSTANCE_SCOPES } from '@n8n/permissions'; import { mock } from 'vitest-mock-extended'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; @@ -35,9 +36,11 @@ describe('RoleService custom role scope whitelist', () => { vi.clearAllMocks(); // Resolve every requested scope as a valid DB scope so that, absent the // whitelist check, the create/update path succeeds - isolating the test to - // the roleType restriction. + // the roleType restriction. Deduped like the real repository, which returns + // distinct rows. scopeRepository.findByList.mockImplementation( - async (slugs: string[]) => slugs.map((slug) => ({ slug })) as unknown as DBScope[], + async (slugs: string[]) => + [...new Set(slugs)].map((slug) => ({ slug })) as unknown as DBScope[], ); roleRepository.save.mockResolvedValue({ slug: 'project:custom-abc123', @@ -49,6 +52,127 @@ describe('RoleService custom role scope whitelist', () => { } as unknown as Role); }); + /** Scope slugs the service handed to `roleRepository.save`. */ + const savedScopeSlugs = () => + (roleRepository.save.mock.calls[0][0] as Role).scopes.map((scope) => scope.slug); + + /** Scope slugs the service handed to `roleRepository.updateRole`. */ + const updatedScopeSlugs = () => + (roleRepository.updateRole.mock.calls[0][1].scopes as DBScope[]).map((scope) => scope.slug); + + describe('mandatory instance scopes', () => { + it('adds them to a global role created with an empty scope list', async () => { + const dto = { + displayName: 'Bare Global Role', + roleType: 'global', + scopes: [], + } as unknown as CreateRoleDto; + + await expect(roleService.createCustomRole(dto)).resolves.toBeDefined(); + expect(savedScopeSlugs()).toEqual([...MANDATORY_INSTANCE_SCOPES]); + }); + + it('adds them to a global role created without them', async () => { + const dto = { + displayName: 'Auditor', + roleType: 'global', + scopes: ['insights:read'], + } as CreateRoleDto; + + await expect(roleService.createCustomRole(dto)).resolves.toBeDefined(); + expect(savedScopeSlugs()).toEqual(['insights:read', ...MANDATORY_INSTANCE_SCOPES]); + }); + + it('restores them when a global role update strips them', async () => { + const dto = { scopes: ['insights:read'] } as UpdateRoleDto; + + await expect( + roleService.updateCustomRole({ + slug: 'global:custom-abc123', + newRole: dto, + userId: 'user-id', + }), + ).resolves.toBeDefined(); + expect(updatedScopeSlugs()).toEqual(['insights:read', ...MANDATORY_INSTANCE_SCOPES]); + }); + + it('does not duplicate them when the caller already sent them', async () => { + const dto = { + displayName: 'Explicit Global Role', + roleType: 'global', + scopes: ['user:list', 'tag:read'], + } as CreateRoleDto; + + await expect(roleService.createCustomRole(dto)).resolves.toBeDefined(); + expect(savedScopeSlugs()).toEqual([...MANDATORY_INSTANCE_SCOPES]); + }); + + it('leaves a created project role untouched', async () => { + const dto = { + displayName: 'Project Role', + roleType: 'project', + scopes: ['workflow:create'], + } as CreateRoleDto; + + await expect(roleService.createCustomRole(dto)).resolves.toBeDefined(); + // `user:list` is not on the project whitelist, so adding it would make + // every project-role write fail. + expect(savedScopeSlugs()).toEqual(['workflow:create']); + }); + + it('leaves an updated project role untouched', async () => { + const dto = { scopes: ['workflow:create'] } as UpdateRoleDto; + + await expect( + roleService.updateCustomRole({ + slug: 'project:custom-abc123', + newRole: dto, + userId: 'user-id', + }), + ).resolves.toBeDefined(); + expect(updatedScopeSlugs()).toEqual(['workflow:create']); + }); + + it('leaves a project role created with an empty scope list empty', async () => { + const dto = { + displayName: 'Empty Project Role', + roleType: 'project', + scopes: [], + } as unknown as CreateRoleDto; + + await expect(roleService.createCustomRole(dto)).resolves.toBeDefined(); + expect(savedScopeSlugs()).toEqual([]); + }); + + it('does not report a repeated input slug as invalid', async () => { + const dto = { + displayName: 'Duplicated Scopes Role', + roleType: 'project', + scopes: ['workflow:create', 'workflow:create'], + } as CreateRoleDto; + + await expect(roleService.createCustomRole(dto)).resolves.toBeDefined(); + expect(savedScopeSlugs()).toEqual(['workflow:create']); + }); + + it('still rejects an unknown scope', async () => { + const dto = { + displayName: 'Unknown Scope Role', + roleType: 'global', + scopes: ['not:a-scope'], + } as CreateRoleDto; + // Resolve only the mandatory scopes, so the requested one comes back invalid. + scopeRepository.findByList.mockResolvedValueOnce( + MANDATORY_INSTANCE_SCOPES.map((slug) => ({ slug })) as unknown as DBScope[], + ); + + await expect(roleService.createCustomRole(dto)).rejects.toThrow( + 'The following scopes are invalid: not:a-scope', + ); + expect(roleRepository.save).not.toHaveBeenCalled(); + }); + }); + describe('createCustomRole', () => { it('rejects a project role carrying a global-only scope', async () => { const dto = { diff --git a/packages/cli/src/services/role.service.ts b/packages/cli/src/services/role.service.ts index 43820e916f1..3e5f34fe106 100644 --- a/packages/cli/src/services/role.service.ts +++ b/packages/cli/src/services/role.service.ts @@ -37,6 +37,7 @@ import { PROJECT_ADMIN_ROLE_SLUG, PROJECT_EDITOR_ROLE_SLUG, PROJECT_VIEWER_ROLE_SLUG, + withMandatoryInstanceScopes, } from '@n8n/permissions'; import { UnexpectedError, UserError } from 'n8n-workflow'; @@ -214,13 +215,21 @@ export class RoleService { return undefined; } - if (scopeSlugs.length === 0) { + // Mandatory options are baseline behaviour for every instance role, so the write + // path adds them even when the caller leaves them out. The editor does the same + // on the form, so a role saved through either surface holds the same scopes. + // Both branches dedup, because `findByList` returns distinct rows and a repeated + // input slug would otherwise be reported as invalid. + const requested = + roleType === 'global' ? withMandatoryInstanceScopes(scopeSlugs) : [...new Set(scopeSlugs)]; + + if (requested.length === 0) { return []; } - const scopes = await this.scopeRepository.findByList(scopeSlugs); - if (scopes.length !== scopeSlugs.length) { - const invalidScopes = scopeSlugs.filter((slug) => !scopes.some((s) => s.slug === slug)); + const scopes = await this.scopeRepository.findByList(requested); + if (scopes.length !== requested.length) { + const invalidScopes = requested.filter((slug) => !scopes.some((s) => s.slug === slug)); throw new Error(`The following scopes are invalid: ${invalidScopes.join(', ')}`); } diff --git a/packages/cli/test/integration/public-api/roles.test.ts b/packages/cli/test/integration/public-api/roles.test.ts index ecdc489e8db..2b6cb713f78 100644 --- a/packages/cli/test/integration/public-api/roles.test.ts +++ b/packages/cli/test/integration/public-api/roles.test.ts @@ -1,6 +1,7 @@ import { testDb } from '@n8n/backend-test-utils'; import { RoleRepository, type User } from '@n8n/db'; import { Container } from '@n8n/di'; +import { MANDATORY_INSTANCE_SCOPES } from '@n8n/permissions'; import { createCustomRoleWithScopeSlugs, createRole } from '@test-integration/db/roles'; import { @@ -12,6 +13,10 @@ import { import { setupTestServer } from '@test-integration/utils'; describe('Roles in Public API', () => { + /** `findByList` does not order its rows, so compare scope sets, not sequences. */ + const expectScopes = (actual: readonly string[], expected: readonly string[]) => + expect([...actual].sort()).toEqual([...expected].sort()); + let owner: User; const testServer = setupTestServer({ endpointGroups: ['publicApi'], @@ -98,10 +103,11 @@ describe('Roles in Public API', () => { systemRole: false, roleType: 'global', licensed: expect.any(Boolean), - scopes: ['user:read'], + scopes: expect.arrayContaining(['user:read', ...MANDATORY_INSTANCE_SCOPES]), createdAt: expect.any(String), updatedAt: expect.any(String), }); + expectScopes(custom.scopes, ['user:read', ...MANDATORY_INSTANCE_SCOPES]); }); const allRolesOf = (body: Record>>) => [ @@ -190,10 +196,11 @@ describe('Roles in Public API', () => { systemRole: false, roleType: 'global', licensed: expect.any(Boolean), - scopes: ['user:read'], + scopes: expect.arrayContaining(['user:read', ...MANDATORY_INSTANCE_SCOPES]), createdAt: expect.any(String), updatedAt: expect.any(String), }); + expectScopes(response.body.scopes, ['user:read', ...MANDATORY_INSTANCE_SCOPES]); }); it('returns a project role', async () => { @@ -288,11 +295,11 @@ describe('Roles in Public API', () => { description: null, systemRole: false, roleType: 'global', - scopes: expect.arrayContaining(['user:read', 'user:list']), + scopes: expect.arrayContaining(['user:read', ...MANDATORY_INSTANCE_SCOPES]), createdAt: expect.any(String), updatedAt: expect.any(String), }); - expect(response.body.scopes).toHaveLength(2); + expectScopes(response.body.scopes, ['user:read', ...MANDATORY_INSTANCE_SCOPES]); }); it('creates a project role and returns 201', async () => { @@ -311,12 +318,22 @@ describe('Roles in Public API', () => { expect(response.body.scopes).toEqual(['workflow:read']); }); - it('creates a role with no scopes and returns 201 (matches internal behaviour)', async () => { + it('creates a global role with no scopes but keeps the mandatory ones', async () => { const response = await testServer .publicApiAgentFor(owner) .post('/roles') .send({ displayName: 'PA empty role', roleType: 'global', scopes: [] }); + expect(response.status).toBe(201); + expectScopes(response.body.scopes, MANDATORY_INSTANCE_SCOPES); + }); + + it('creates a project role with no scopes and adds nothing', async () => { + const response = await testServer + .publicApiAgentFor(owner) + .post('/roles') + .send({ displayName: 'PA empty project role', roleType: 'project', scopes: [] }); + expect(response.status).toBe(201); expect(response.body.scopes).toEqual([]); }); @@ -485,15 +502,15 @@ describe('Roles in Public API', () => { description: 'An updated description', systemRole: false, roleType: 'global', - scopes: expect.arrayContaining(['user:read', 'user:list']), + scopes: expect.arrayContaining(['user:read', ...MANDATORY_INSTANCE_SCOPES]), createdAt: expect.any(String), updatedAt: expect.any(String), }); - expect(response.body.scopes).toHaveLength(2); + expectScopes(response.body.scopes, ['user:read', ...MANDATORY_INSTANCE_SCOPES]); }); it('replaces scopes entirely rather than merging them with the existing set', async () => { - const role = await createGlobalRole('PA put scopes', ['user:read', 'user:list']); + const role = await createGlobalRole('PA put scopes', ['user:read', 'insights:read']); const response = await testServer .publicApiAgentFor(owner) @@ -501,7 +518,20 @@ describe('Roles in Public API', () => { .send(fullBody({ displayName: role.displayName, scopes: ['user:read'] })); expect(response.status).toBe(200); - expect(response.body.scopes).toEqual(['user:read']); + // The mandatory scopes survive; `insights:read` does not. + expectScopes(response.body.scopes, ['user:read', ...MANDATORY_INSTANCE_SCOPES]); + }); + + it('keeps the mandatory scopes when the update sends an empty list', async () => { + const role = await createGlobalRole('PA put empty scopes', ['user:read', 'insights:read']); + + const response = await testServer + .publicApiAgentFor(owner) + .put(`/roles/${role.slug}`) + .send(fullBody({ displayName: role.displayName, scopes: [] })); + + expect(response.status).toBe(200); + expectScopes(response.body.scopes, MANDATORY_INSTANCE_SCOPES); }); it('clears an existing description by sending null', async () => { @@ -541,7 +571,9 @@ describe('Roles in Public API', () => { expect(response.status).toBe(200); expect(response.body.displayName).toBe(role.displayName); expect(response.body.description).toBeNull(); - expect(response.body.scopes).toEqual(expect.arrayContaining(['user:read', 'user:list'])); + expect(response.body.scopes).toEqual( + expect.arrayContaining(['user:read', ...MANDATORY_INSTANCE_SCOPES]), + ); }); it('rejects a body missing a required field with 400', async () => { @@ -751,7 +783,7 @@ describe('Roles in Public API', () => { description: null, systemRole: false, roleType: 'global', - scopes: expect.arrayContaining(['user:read', 'user:list']), + scopes: expect.arrayContaining(['user:read', ...MANDATORY_INSTANCE_SCOPES]), createdAt: expect.any(String), updatedAt: expect.any(String), }); diff --git a/packages/cli/test/integration/services/role.service.test.ts b/packages/cli/test/integration/services/role.service.test.ts index 2de5931921a..c4d1ca9b014 100644 --- a/packages/cli/test/integration/services/role.service.test.ts +++ b/packages/cli/test/integration/services/role.service.test.ts @@ -4,7 +4,7 @@ import { testDb } from '@n8n/backend-test-utils'; import { ProjectRepository } from '@n8n/db'; import { RoleMappingRuleRepository, RoleRepository, UserRepository } from '@n8n/db'; import { Container } from '@n8n/di'; -import { ALL_ROLES } from '@n8n/permissions'; +import { ALL_ROLES, MANDATORY_INSTANCE_SCOPES } from '@n8n/permissions'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; @@ -899,6 +899,12 @@ describe('RoleService', () => { licensed: expect.any(Boolean), }); + // Every instance role carries the mandatory options, whatever was sent. + // `findByList` does not order its rows, so compare sets, not sequences. + expect([...result.scopes].sort()).toEqual( + ['user:read', 'role:read', ...MANDATORY_INSTANCE_SCOPES].sort(), + ); + // Verify slug was generated with the global namespace expect(result.slug).toMatch(/^global:test-global-role-[a-z0-9]{6}$/); @@ -1098,6 +1104,9 @@ describe('RoleService', () => { scopes: expect.arrayContaining(updateRoleDto.scopes as string[]), }); + // The update replaces the stored scopes, but the mandatory options survive. + expect([...result.scopes].sort()).toEqual(['user:read', ...MANDATORY_INSTANCE_SCOPES].sort()); + const updatedRole = await roleRepository.findBySlug(existingRole.slug); expect(updatedRole?.roleType).toBe('global'); expect(updatedRole?.displayName).toBe(updateRoleDto.displayName); @@ -1164,15 +1173,16 @@ describe('RoleService', () => { expect(result.displayName).toBe(updateRoleDto.displayName); }); - it('should update role with empty scopes array', async () => { + it('should clear the scopes of a project role with an empty scopes array', async () => { // // ARRANGE // const testScopes = await createTestScopes(); - const existingRole = await createCustomRoleWithScopes([ - testScopes.readScope, - testScopes.writeScope, - ]); + // `resolveScopes` reads the role type off the slug, so it has to be namespaced. + const existingRole = await createCustomRoleWithScopes( + [testScopes.readScope, testScopes.writeScope], + { slug: `project:empty-scopes-${Math.random().toString(36).substring(7)}` }, + ); const updateRoleDto: UpdateRoleDto = { scopes: [], @@ -1193,6 +1203,38 @@ describe('RoleService', () => { expect(result.scopes).toEqual([]); }); + it('should keep the mandatory scopes on a global role updated with an empty scopes array', async () => { + // + // ARRANGE + // + const testScopes = await createTestScopes(); + const existingRole = await createCustomRoleWithScopes( + [testScopes.readScope, testScopes.writeScope], + { + slug: `global:empty-scopes-${Math.random().toString(36).substring(7)}`, + roleType: 'global', + }, + ); + + const updateRoleDto: UpdateRoleDto = { + scopes: [], + }; + + // + // ACT + // + const result = await roleService.updateCustomRole({ + slug: existingRole.slug, + newRole: updateRoleDto, + userId: 'test-user-id', + }); + + // + // ASSERT + // + expect([...result.scopes].sort()).toEqual([...MANDATORY_INSTANCE_SCOPES].sort()); + }); + it('should throw error when role does not exist', async () => { // // ARRANGE diff --git a/packages/frontend/editor-ui/src/features/roles/instance/InstanceRoleView.test.ts b/packages/frontend/editor-ui/src/features/roles/instance/InstanceRoleView.test.ts index 5b95da9a4fd..23d2b88db98 100644 --- a/packages/frontend/editor-ui/src/features/roles/instance/InstanceRoleView.test.ts +++ b/packages/frontend/editor-ui/src/features/roles/instance/InstanceRoleView.test.ts @@ -42,7 +42,7 @@ const mockCustomRole = { displayName: 'Support', slug: 'support', description: 'A custom instance role', - // The mandatory options every instance role carries — see instanceRoleScopes.ts. + // The mandatory options every instance role carries — see MANDATORY_INSTANCE_OPTIONS. scopes: ['user:list', 'tag:read', 'tag:list'], licensed: true, systemRole: false, diff --git a/packages/frontend/editor-ui/src/features/roles/instance/instanceRoleScopes.ts b/packages/frontend/editor-ui/src/features/roles/instance/instanceRoleScopes.ts index 25a385b65d3..95977132fc0 100644 --- a/packages/frontend/editor-ui/src/features/roles/instance/instanceRoleScopes.ts +++ b/packages/frontend/editor-ui/src/features/roles/instance/instanceRoleScopes.ts @@ -8,8 +8,13 @@ */ import { type BaseTextKey } from '@n8n/i18n'; -import { GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS, type Scope } from '@n8n/permissions'; +import { + GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS, + isMandatoryInstanceOption, + type Scope, +} from '@n8n/permissions'; export { GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS as INSTANCE_SCOPE_GROUPS } from '@n8n/permissions'; +export { withMandatoryInstanceScopes } from '@n8n/permissions'; export type InstanceResource = keyof typeof GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS; @@ -164,55 +169,29 @@ export const ALL_INSTANCE_SCOPES: Scope[] = [ ...new Set(INSTANCE_SCOPE_GROUP_LIST.flatMap((g) => g.options.flatMap((o) => o.scopes))), ]; -type MandatoryInstanceOption = { - resource: InstanceResource; - option: string; - /** Overrides the option's own description where the "why it's locked" wording differs. */ - tooltipKey?: BaseTextKey; -}; - /** - * Options every instance role carries regardless of what is saved — the default - * Member role already grants them, so they are baseline behaviour rather than - * something a custom role opts into. Rendered checked and disabled. - * `withMandatoryInstanceScopes` is applied to the form (and on save), not the - * persisted snapshot, so a stored role missing these scopes stays unsaved until - * the next save. + * Tooltip overrides for the mandatory options declared in `@n8n/permissions`. + * The wording that explains *why* an option is locked lives here because + * `@n8n/permissions` must stay free of i18n types. */ -const MANDATORY_INSTANCE_OPTIONS: readonly MandatoryInstanceOption[] = [ - { resource: 'user', option: 'View', tooltipKey: 'instanceRoles.option.mandatory' }, - { resource: 'tag', option: 'View' }, // falls back to instanceRoles.description.tag.view -]; +const MANDATORY_OPTION_TOOLTIP_KEYS: Partial< + Record> +> = { + user: { View: 'instanceRoles.option.mandatory' }, + // tag View falls back to instanceRoles.description.tag.view +}; /** Tooltip key for a mandatory option, or undefined when the option is not mandatory. */ export function mandatoryOptionTooltipKey( resource: InstanceResource, option: InstanceScopeOption, ): BaseTextKey | undefined { - const entry = MANDATORY_INSTANCE_OPTIONS.find( - (m) => m.resource === resource && m.option === option.key, - ); - return entry ? (entry.tooltipKey ?? option.descriptionKey) : undefined; + if (!isOptionMandatory(resource, option)) return undefined; + return MANDATORY_OPTION_TOOLTIP_KEYS[resource]?.[option.key] ?? option.descriptionKey; } export function isOptionMandatory(resource: InstanceResource, option: InstanceScopeOption) { - return MANDATORY_INSTANCE_OPTIONS.some((m) => m.resource === resource && m.option === option.key); -} - -/** Derived from the group list so a typo in the table contributes nothing silently. */ -const MANDATORY_INSTANCE_SCOPES: readonly Scope[] = [ - ...new Set( - INSTANCE_SCOPE_GROUP_LIST.flatMap((group) => - group.options - .filter((option) => isOptionMandatory(group.resource, option)) - .flatMap((option) => option.scopes), - ), - ), -]; - -/** Unions in the mandatory scopes (see `isOptionMandatory`) on top of an already-filtered scope list. */ -export function withMandatoryInstanceScopes(scopes: readonly string[]): string[] { - return [...new Set([...scopes, ...MANDATORY_INSTANCE_SCOPES])]; + return isMandatoryInstanceOption(resource, option.key); } export type OptionState = 'checked' | 'indeterminate' | 'unchecked'; From 873948b8385b464bf6d6d89e5ff11177f86bc466 Mon Sep 17 00:00:00 2001 From: RomanDavydchuk Date: Fri, 18 Sep 2026 09:17:50 +0000 Subject: [PATCH 06/67] feat(core): Filter out MCP servers from the registry based on instance capabilities (#38867) --- .../__tests__/mcp-registry-api.client.test.ts | 36 +++++++++++++++---- .../mcp-registry-capabilities.test.ts | 18 ++++++++++ .../__tests__/mcp-registry.service.test.ts | 30 +++++++++++++++- .../registry/mcp-registry-api.client.ts | 3 +- .../registry/mcp-registry-capabilities.ts | 25 +++++++++++++ .../registry/mcp-registry-server.entity.ts | 1 + .../registry/mcp-registry.service.ts | 28 +++++++++++---- .../registry/mcp-registry.types.ts | 1 + 8 files changed, 127 insertions(+), 15 deletions(-) create mode 100644 packages/cli/src/modules/mcp-registry/registry/__tests__/mcp-registry-capabilities.test.ts create mode 100644 packages/cli/src/modules/mcp-registry/registry/mcp-registry-capabilities.ts diff --git a/packages/cli/src/modules/mcp-registry/registry/__tests__/mcp-registry-api.client.test.ts b/packages/cli/src/modules/mcp-registry/registry/__tests__/mcp-registry-api.client.test.ts index 04cb0a4adaf..74fd4f39c6e 100644 --- a/packages/cli/src/modules/mcp-registry/registry/__tests__/mcp-registry-api.client.test.ts +++ b/packages/cli/src/modules/mcp-registry/registry/__tests__/mcp-registry-api.client.test.ts @@ -143,7 +143,7 @@ describe('McpRegistryApiClient', () => { expect(mockPaginatedRequest).toHaveBeenCalledWith( PRODUCTION_URL, { - version: 3, + version: 4, pagination: { page: 1, pageSize: 25 }, }, { throwOnError: true }, @@ -245,6 +245,30 @@ describe('McpRegistryApiClient', () => { expect(result[0].tools[1].annotations).toBeUndefined(); }); + it.each([ + ['a missing value', undefined, undefined], + ['null', null, undefined], + ['an empty array', [], []], + ['a capability array', ['unsupported-capability'], ['unsupported-capability']], + ])('should parse requiredCapabilities from %s', async (_, requiredCapabilities, expected) => { + mockPaginatedRequest.mockResolvedValue([{ ...notionMockServer, requiredCapabilities }]); + + const result = await client.fetchAllServers(); + + expect(result[0].requiredCapabilities).toEqual(expected); + }); + + it('should skip a server with malformed requiredCapabilities', async () => { + mockPaginatedRequest.mockResolvedValue([ + notionMockServer, + { ...notionMockServer, slug: 'malformed', requiredCapabilities: [1] }, + ]); + + const result = await client.fetchAllServers(); + + expect(result).toEqual([notionMockServer]); + }); + it('should keep only OAuth2 credential options', async () => { mockPaginatedRequest.mockResolvedValue([githubUsesCredentialsMockServer]); vi.mocked(credentialTypes.getParentTypes).mockImplementation((credentialType) => @@ -297,7 +321,7 @@ describe('McpRegistryApiClient', () => { expect(mockPaginatedRequest).toHaveBeenCalledWith( PRODUCTION_URL, { - version: 3, + version: 4, fields: ['slug', 'version', 'updatedAt'], pagination: { page: 1, pageSize: 500 }, }, @@ -339,7 +363,7 @@ describe('McpRegistryApiClient', () => { expect(mockPaginatedRequest).toHaveBeenCalledWith( PRODUCTION_URL, { - version: 3, + version: 4, filters: { slug: { $in: ['server-a', 'server-b', 'server-c'], @@ -392,7 +416,7 @@ describe('McpRegistryApiClient', () => { 1, PRODUCTION_URL, { - version: 3, + version: 4, filters: { slug: { $in: slugs.slice(0, 100), @@ -408,7 +432,7 @@ describe('McpRegistryApiClient', () => { 2, PRODUCTION_URL, { - version: 3, + version: 4, filters: { slug: { $in: slugs.slice(100, 200), @@ -424,7 +448,7 @@ describe('McpRegistryApiClient', () => { 3, PRODUCTION_URL, { - version: 3, + version: 4, filters: { slug: { $in: slugs.slice(200, 250), diff --git a/packages/cli/src/modules/mcp-registry/registry/__tests__/mcp-registry-capabilities.test.ts b/packages/cli/src/modules/mcp-registry/registry/__tests__/mcp-registry-capabilities.test.ts new file mode 100644 index 00000000000..3ef74afd1bf --- /dev/null +++ b/packages/cli/src/modules/mcp-registry/registry/__tests__/mcp-registry-capabilities.test.ts @@ -0,0 +1,18 @@ +import type { GlobalConfig } from '@n8n/config'; +import { mock } from 'vitest-mock-extended'; + +import { McpRegistryCapabilities } from '../mcp-registry-capabilities'; + +describe('McpRegistryCapabilities', () => { + it.each([ + ['default', undefined, true], + ['default', ['n8n-cloud'], false], + ['cloud', ['n8n-cloud'], true], + ['cloud', ['n8n-cloud', 'unsupported-capability'], false], + ])('checks capabilities for a %s deployment', (deploymentType, required, expected) => { + const globalConfig = mock({ deployment: { type: deploymentType } }); + const capabilities = new McpRegistryCapabilities(globalConfig); + + expect(capabilities.supports(required)).toBe(expected); + }); +}); diff --git a/packages/cli/src/modules/mcp-registry/registry/__tests__/mcp-registry.service.test.ts b/packages/cli/src/modules/mcp-registry/registry/__tests__/mcp-registry.service.test.ts index 708472a2f61..0fbbfce88c0 100644 --- a/packages/cli/src/modules/mcp-registry/registry/__tests__/mcp-registry.service.test.ts +++ b/packages/cli/src/modules/mcp-registry/registry/__tests__/mcp-registry.service.test.ts @@ -1,4 +1,5 @@ import type { Logger } from '@n8n/backend-common'; +import type { GlobalConfig } from '@n8n/config'; import type { InstanceSettings } from 'n8n-core'; import { mock } from 'vitest-mock-extended'; @@ -10,6 +11,7 @@ import { resolveMcpRegistryConnection } from '../../mcp-registry-connection'; import { McpRegistryNodeLoader } from '../../mcp-registry-node-loader'; import { MCP_REGISTRY_PACKAGE_NAME } from '../../node-description-transform'; import type { McpRegistryApiClient, McpRegistryServerMetadata } from '../mcp-registry-api.client'; +import { McpRegistryCapabilities } from '../mcp-registry-capabilities'; import type { McpRegistryServerEntity } from '../mcp-registry-server.entity'; import type { McpRegistryServerRepository } from '../mcp-registry-server.repository'; import { McpRegistryService } from '../mcp-registry.service'; @@ -31,6 +33,10 @@ function createService(options: CreateServiceOptions = {}) { const logger = mock({ scoped: vi.fn().mockReturnThis() }); const repository = mock(); const apiClient = mock(); + const globalConfig = mock({ + deployment: { type: 'default' }, + }); + const capabilities = new McpRegistryCapabilities(globalConfig); const instanceSettings = mock({ instanceType: options.instanceType ?? 'main', }); @@ -72,6 +78,7 @@ function createService(options: CreateServiceOptions = {}) { logger, repository, apiClient, + capabilities, instanceSettings, loadNodesAndCredentials, push, @@ -137,6 +144,22 @@ describe('McpRegistryService', () => { expect(missing).toBeUndefined(); }); + it('excludes servers that require an unsupported capability', async () => { + const unsupportedServer: McpRegistryServer = { + ...linearMockServer, + requiredCapabilities: ['unsupported-capability'], + }; + const { service } = createService({ + storedServers: [notionMockServer, unsupportedServer], + }); + + expect(await service.getAll()).toEqual([notionMockServer]); + expect(await service.get(unsupportedServer.slug)).toBeUndefined(); + expect(await service.getBySlugs([notionMockServer.slug, unsupportedServer.slug])).toEqual([ + notionMockServer, + ]); + }); + it('returns empty array for getBySlugs when input is empty', async () => { const { service, repository } = createService(); @@ -289,12 +312,17 @@ describe('McpRegistryService', () => { it('refreshFromApi fetches all servers when no data is persisted', async () => { const { service, apiClient, repository } = createService({ storedServers: null }); + const unsupportedServer: McpRegistryServer = { + ...notionMockServer, + requiredCapabilities: ['unsupported-capability'], + }; + apiClient.fetchAllServers.mockResolvedValue([unsupportedServer]); await service.refreshFromApi(); expect(apiClient.fetchAllServers).toHaveBeenCalledTimes(1); expect(apiClient.fetchServersMetadata).not.toHaveBeenCalled(); - expect(repository.upsert).toHaveBeenCalledTimes(1); + expect(repository.upsert).toHaveBeenCalledWith([toEntity(unsupportedServer)], ['slug']); }); it('refreshFromApi stops before the write when the signal aborts during the fetch', async () => { diff --git a/packages/cli/src/modules/mcp-registry/registry/mcp-registry-api.client.ts b/packages/cli/src/modules/mcp-registry/registry/mcp-registry-api.client.ts index d3afa156023..655d9053378 100644 --- a/packages/cli/src/modules/mcp-registry/registry/mcp-registry-api.client.ts +++ b/packages/cli/src/modules/mcp-registry/registry/mcp-registry-api.client.ts @@ -18,8 +18,9 @@ const STRAPI_ARRAY_LIMIT = 100; /** Version history: * 2 - introduced authType: `usesCredentials` field * 3 - introduced remotes[].type: `streamable-http-templated` + * 4 - introduced `requiredCapabilities` field */ -const STRAPI_API_VERSION = 3; +const STRAPI_API_VERSION = 4; @Service() export class McpRegistryApiClient { diff --git a/packages/cli/src/modules/mcp-registry/registry/mcp-registry-capabilities.ts b/packages/cli/src/modules/mcp-registry/registry/mcp-registry-capabilities.ts new file mode 100644 index 00000000000..1fc4f7b8075 --- /dev/null +++ b/packages/cli/src/modules/mcp-registry/registry/mcp-registry-capabilities.ts @@ -0,0 +1,25 @@ +import { GlobalConfig } from '@n8n/config'; +import { Service } from '@n8n/di'; + +// Add capabilities here when they do not depend on instance configuration. +const BASE_CAPABILITIES: readonly string[] = []; + +const N8N_CLOUD_CAPABILITY = 'n8n-cloud'; + +@Service() +export class McpRegistryCapabilities { + private readonly supportedCapabilities = new Set(BASE_CAPABILITIES); + + constructor(globalConfig: GlobalConfig) { + if (globalConfig.deployment.type === 'cloud') { + this.supportedCapabilities.add(N8N_CLOUD_CAPABILITY); + } + } + + supports(requiredCapabilities?: string[]): boolean { + return ( + requiredCapabilities?.every((capability) => this.supportedCapabilities.has(capability)) ?? + true + ); + } +} diff --git a/packages/cli/src/modules/mcp-registry/registry/mcp-registry-server.entity.ts b/packages/cli/src/modules/mcp-registry/registry/mcp-registry-server.entity.ts index 9ba486cea26..6bc39cda452 100644 --- a/packages/cli/src/modules/mcp-registry/registry/mcp-registry-server.entity.ts +++ b/packages/cli/src/modules/mcp-registry/registry/mcp-registry-server.entity.ts @@ -25,6 +25,7 @@ export type McpRegistryServerData = { }>; websiteUrl?: string; tags?: string[]; + requiredCapabilities?: string[]; extendsCredential?: { extends: string; authUrl?: string | null; diff --git a/packages/cli/src/modules/mcp-registry/registry/mcp-registry.service.ts b/packages/cli/src/modules/mcp-registry/registry/mcp-registry.service.ts index 1d580c48c60..bf33982aabd 100644 --- a/packages/cli/src/modules/mcp-registry/registry/mcp-registry.service.ts +++ b/packages/cli/src/modules/mcp-registry/registry/mcp-registry.service.ts @@ -12,6 +12,7 @@ import { McpRegistryServerRepository } from './mcp-registry-server.repository'; import { McpRegistryNodeLoader } from '../mcp-registry-node-loader'; import type { McpRegistryServerMetadata } from './mcp-registry-api.client'; import { McpRegistryApiClient } from './mcp-registry-api.client'; +import { McpRegistryCapabilities } from './mcp-registry-capabilities'; import { listMcpRegistryServers, searchMcpRegistryServers, @@ -27,6 +28,7 @@ export class McpRegistryService { private readonly logger: Logger, private readonly repository: McpRegistryServerRepository, private readonly apiClient: McpRegistryApiClient, + private readonly capabilities: McpRegistryCapabilities, private readonly instanceSettings: InstanceSettings, private readonly loadNodesAndCredentials: LoadNodesAndCredentials, private readonly push: Push, @@ -50,15 +52,18 @@ export class McpRegistryService { async getAll({ includeDeprecated = false, }: { includeDeprecated?: boolean } = {}): Promise { - const entities = includeDeprecated - ? await this.repository.find() - : await this.repository.findBy({ status: 'active' }); - return entities.map(fromEntity); + const servers = await this.getStoredServers(includeDeprecated); + return servers.filter(({ requiredCapabilities }) => + this.capabilities.supports(requiredCapabilities), + ); } async get(slug: string): Promise { const entity = await this.repository.findOneBy({ slug }); - return entity ? fromEntity(entity) : undefined; + if (!entity) return undefined; + + const server = fromEntity(entity); + return this.capabilities.supports(server.requiredCapabilities) ? server : undefined; } async getBySlugs(slugs: string[]): Promise { @@ -67,7 +72,9 @@ export class McpRegistryService { } const entities = await this.repository.findBy(slugs.map((slug) => ({ slug }))); - return entities.map(fromEntity); + return entities + .map(fromEntity) + .filter(({ requiredCapabilities }) => this.capabilities.supports(requiredCapabilities)); } /** @@ -102,7 +109,7 @@ export class McpRegistryService { * signal aborts before the write starts. The signal cancels the API requests. */ async refreshFromApi(signal?: AbortSignal): Promise { - const existingServers = await this.getAll({ includeDeprecated: true }); + const existingServers = await this.getStoredServers(true); let updatedServers: McpRegistryServer[]; if (existingServers.length === 0) { updatedServers = await this.apiClient.fetchAllServers(signal); @@ -125,6 +132,13 @@ export class McpRegistryService { this.logger.debug('MCP registry refreshed', { serverCount: updatedServers.length }); } + private async getStoredServers(includeDeprecated: boolean): Promise { + const entities = includeDeprecated + ? await this.repository.find() + : await this.repository.findBy({ status: 'active' }); + return entities.map(fromEntity); + } + private async refreshUpdatedServers( existingServers: McpRegistryServer[], signal?: AbortSignal, diff --git a/packages/cli/src/modules/mcp-registry/registry/mcp-registry.types.ts b/packages/cli/src/modules/mcp-registry/registry/mcp-registry.types.ts index e7596a0566b..22441838170 100644 --- a/packages/cli/src/modules/mcp-registry/registry/mcp-registry.types.ts +++ b/packages/cli/src/modules/mcp-registry/registry/mcp-registry.types.ts @@ -109,6 +109,7 @@ const mcpRegistryServerBaseSchema = z.object({ isOfficial: z.boolean(), origin: z.literal('registry'), status: z.enum(serverStatuses), + requiredCapabilities: optionalField(z.array(z.string())), // The API returns either a bare array or a `{ data }` envelope, and omits // `data` entirely when there are no tags. Anything stricter drops the whole // server over optional metadata. From 3fdf46e530e6e53f626d7ea7b0a580deebc249be Mon Sep 17 00:00:00 2001 From: Declan Carroll Date: Fri, 18 Sep 2026 09:18:52 +0000 Subject: [PATCH 07/67] test: Keep local sandbox TLS files removable (no-changelog) (#39047) --- packages/testing/containers/services/sandbox.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/testing/containers/services/sandbox.ts b/packages/testing/containers/services/sandbox.ts index 7cb21a8be90..885ca735b2c 100644 --- a/packages/testing/containers/services/sandbox.ts +++ b/packages/testing/containers/services/sandbox.ts @@ -1,5 +1,5 @@ import { execFile } from 'node:child_process'; -import { chmodSync, mkdtempSync } from 'node:fs'; +import { chmodSync, mkdtempSync, statSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { promisify } from 'node:util'; @@ -120,6 +120,7 @@ async function generateMtlsCerts( const tlsDir = mkdtempSync(join(tmpdir(), `${projectName}-sandbox-tls-`)); ctx?.registerPath?.(tlsDir); chmodSync(tlsDir, 0o755); + const { uid: hostUid, gid: hostGid } = statSync(tlsDir); const { consumer, throwWithLogs } = createSilentLogConsumer(); try { @@ -132,8 +133,8 @@ async function generateMtlsCerts( '-c', [ 'bootstrap-mtls.sh --out-dir /tls --api-san sandbox-api --control-san-prefix sandbox-runner --world-readable', - 'chown -R sandbox-api:sandbox-api /tls/api', 'chmod -R a+rX /tls', + `chown -R ${hostUid}:${hostGid} /tls`, `echo ${CERT_GEN_SENTINEL}`, ].join(' && '), ]) From 8e0c08dff2bf35ca95f72dcfcf4ac6ee9eefaae2 Mon Sep 17 00:00:00 2001 From: Savelii Date: Fri, 18 Sep 2026 09:23:24 +0000 Subject: [PATCH 08/67] ci: Run the encryption rollout test only in the public repository (no-changelog) (#39048) Co-authored-by: Claude Fable 5 --- .github/workflows/encryption-upgrade-test.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/encryption-upgrade-test.yml b/.github/workflows/encryption-upgrade-test.yml index 6d47fe44978..14d4178feba 100644 --- a/.github/workflows/encryption-upgrade-test.yml +++ b/.github/workflows/encryption-upgrade-test.yml @@ -46,6 +46,9 @@ concurrency: jobs: encryption-cycle: name: ${{ matrix.mode }} cycle (${{ matrix.db }}) + # The mirror repository must not run this suite: its runners run out of + # disk during the image build, and the cron would fire twice. + if: github.repository == 'n8n-io/n8n' runs-on: ubuntu-latest timeout-minutes: 60 strategy: From b37fdc2d3a5bf0706eda922e1a35ba24b9385d09 Mon Sep 17 00:00:00 2001 From: oleg Date: Fri, 18 Sep 2026 09:34:46 +0000 Subject: [PATCH 09/67] feat(editor): Add inline credential setup to n8n Assistant (no-changelog) (#38684) --- packages/@n8n/agents/src/index.ts | 1 + packages/@n8n/agents/src/skills/index.ts | 1 + packages/@n8n/agents/src/skills/tools.ts | 8 +- packages/@n8n/agents/src/skills/types.ts | 3 + .../__tests__/agent-run-reducer.test.ts | 27 + .../src/schemas/agent-run-reducer.ts | 5 + .../src/schemas/instance-ai.schema.ts | 2 + .../skills/post-build-flow/SKILL.md | 3 + .../skills/workflow-builder/SKILL.md | 4 +- .../system-prompt.setup-panel.test.ts | 3 +- .../instance-ai/src/agent/system-prompt.ts | 2 +- .../materialize-runtime-skills.test.ts | 6 +- .../src/skills/materialize-runtime-skills.ts | 6 +- .../instance-ai/src/tools/credentials.tool.ts | 2 +- .../instance-ai/src/tools/workflows.tool.ts | 2 +- .../src/tools/workflows/setup-panel-state.ts | 2 +- .../workflow-loop/__tests__/guidance.test.ts | 2 +- .../instance-ai/src/workflow-loop/guidance.ts | 2 +- .../@n8n/telemetry/src/events/credentials.ts | 26 + .../@n8n/telemetry/src/events/instance-ai.ts | 41 + .../@n8n/telemetry/src/events/workflow.ts | 10 + .../SetupConnection.stories.ts | 29 + .../SetupConnection.test.ts | 62 ++ .../SetupConnection.types.ts | 23 + .../N8nSetupConnection/SetupConnection.vue | 153 +++ .../N8nSetupPanel/SetupPanel.stories.ts | 356 +++++++ .../N8nSetupPanel/SetupPanel.test.ts | 239 +++++ .../N8nSetupPanel/SetupPanel.types.ts | 25 + .../components/N8nSetupPanel/SetupPanel.vue | 444 +++++++++ .../design-system/src/components/index.ts | 4 + .../@n8n/design-system/src/locale/lang/en.ts | 11 + .../frontend/@n8n/i18n/src/locales/en.json | 36 +- .../useInstanceAiEditorCapability.ts | 2 + .../src/app/composables/useRunWorkflow.ts | 34 +- .../src/app/composables/useRunWorkflowApi.ts | 46 + .../editor-ui/src/app/constants/durations.ts | 3 + .../src/app/constants/injectionKeys.ts | 4 + .../src/app/constants/localStorage.ts | 10 + .../editor-ui/src/app/constants/nodeTypes.ts | 9 + .../useChatHubMarkdownOptions.test.ts | 8 +- .../composables/useChatHubMarkdownOptions.ts | 1 + .../ai/instanceAi/InstanceAiThreadView.vue | 8 +- .../__tests__/InstanceAiThreadView.test.ts | 36 + .../__tests__/canvasPreview.utils.test.ts | 26 + .../createThreadComponentRenderer.ts | 2 + .../instanceAi.threadRuntime.test.ts | 62 ++ .../__tests__/setupPanelGroups.test.ts | 65 ++ .../__tests__/useSetupPanelActions.test.ts | 156 ++- .../__tests__/useSetupPanelExecution.test.ts | 422 ++++++++ .../__tests__/useSetupPanelState.test.ts | 48 +- .../__tests__/useSetupPanelTelemetry.test.ts | 170 ++++ .../ai/instanceAi/canvasPreview.utils.ts | 22 +- .../InstanceAiConfirmationPanel.vue | 19 + .../components/InstanceAiConversation.vue | 30 +- .../components/InstanceAiWorkflowPreview.vue | 3 + .../setupPanel/InstanceAiSetupCredential.vue | 751 +++++++++++++++ .../setupPanel/InstanceAiSetupPanel.vue | 907 ++++++++++++++---- .../setupPanel/InstanceAiSetupPanelDetail.vue | 262 ++--- .../InstanceAiSetupCredential.test.ts | 843 ++++++++++++++++ .../InstanceAiSetupPanel.interactions.test.ts | 473 ++++++++- .../__tests__/InstanceAiSetupPanel.test.ts | 640 +++++++++++- .../composables/useInstanceAiHandoff.ts | 3 +- .../composables/useSetupPanelActions.ts | 260 +++-- .../composables/useSetupPanelDocument.ts | 52 + .../composables/useSetupPanelExecution.ts | 238 +++++ .../composables/useSetupPanelState.ts | 46 +- .../composables/useSetupPanelTelemetry.ts | 163 ++++ .../ai/instanceAi/instanceAi.threadRuntime.ts | 16 + .../ai/instanceAi/setupPanelGroups.ts | 64 ++ .../CredentialEdit/CredentialInputs.test.ts | 90 ++ .../CredentialEdit/CredentialInputs.vue | 87 +- .../TemplatedAuthSimpleView.vue | 25 +- .../components/NodeCredentials.test.ts | 22 +- .../components/NodeCredentials.vue | 35 +- .../__tests__/useCredentialOAuth.test.ts | 44 + .../composables/useCredentialForm.ts | 5 +- .../composables/useCredentialOAuth.ts | 41 +- .../features/credentials/credentials.store.ts | 13 +- .../composables/useQuickConnect.ts | 16 +- .../AgentSelectorParameterInput.test.ts | 40 + .../AgentSelectorParameterInput.vue | 25 +- .../AssignmentCollection.vue | 11 +- .../parameters/components/ParameterInput.vue | 19 +- .../components/ParameterInputExpanded.vue | 3 +- .../components/ParameterInputList.vue | 1 + .../ResourceLocator/ResourceLocator.test.ts | 25 + .../ResourceLocator/ResourceLocator.vue | 24 +- .../ResourceLocatorDropdown.vue | 6 +- .../WorkflowSelectorParameterInput.test.ts | 61 ++ .../WorkflowSelectorParameterInput.vue | 31 +- .../composables/useWorkflowSetupItems.test.ts | 177 +++- .../composables/useWorkflowSetupItems.ts | 138 ++- .../setupPanel/setupPanel.utils.test.ts | 18 + .../features/setupPanel/setupPanel.utils.ts | 10 +- .../parts/CanvasNodeStatusIcons.test.ts | 52 +- .../parts/CanvasNodeStatusIcons.vue | 37 +- .../credentials/ClearbitApi.credentials.ts | 1 + .../credentials/NotionApi.credentials.ts | 1 + .../credentials/SlackApi.credentials.ts | 2 + .../credentials/TelegramApi.credentials.ts | 1 + .../nodes/Slack/V2/GenericFunctions.ts | 3 + .../Slack/test/v2/GenericFunctions.test.ts | 39 + 102 files changed, 7743 insertions(+), 804 deletions(-) create mode 100644 packages/frontend/@n8n/design-system/src/components/N8nSetupConnection/SetupConnection.stories.ts create mode 100644 packages/frontend/@n8n/design-system/src/components/N8nSetupConnection/SetupConnection.test.ts create mode 100644 packages/frontend/@n8n/design-system/src/components/N8nSetupConnection/SetupConnection.types.ts create mode 100644 packages/frontend/@n8n/design-system/src/components/N8nSetupConnection/SetupConnection.vue create mode 100644 packages/frontend/@n8n/design-system/src/components/N8nSetupPanel/SetupPanel.stories.ts create mode 100644 packages/frontend/@n8n/design-system/src/components/N8nSetupPanel/SetupPanel.test.ts create mode 100644 packages/frontend/@n8n/design-system/src/components/N8nSetupPanel/SetupPanel.types.ts create mode 100644 packages/frontend/@n8n/design-system/src/components/N8nSetupPanel/SetupPanel.vue create mode 100644 packages/frontend/editor-ui/src/app/composables/useRunWorkflowApi.ts create mode 100644 packages/frontend/editor-ui/src/features/ai/instanceAi/__tests__/setupPanelGroups.test.ts create mode 100644 packages/frontend/editor-ui/src/features/ai/instanceAi/__tests__/useSetupPanelExecution.test.ts create mode 100644 packages/frontend/editor-ui/src/features/ai/instanceAi/__tests__/useSetupPanelTelemetry.test.ts create mode 100644 packages/frontend/editor-ui/src/features/ai/instanceAi/components/setupPanel/InstanceAiSetupCredential.vue create mode 100644 packages/frontend/editor-ui/src/features/ai/instanceAi/components/setupPanel/__tests__/InstanceAiSetupCredential.test.ts create mode 100644 packages/frontend/editor-ui/src/features/ai/instanceAi/composables/useSetupPanelDocument.ts create mode 100644 packages/frontend/editor-ui/src/features/ai/instanceAi/composables/useSetupPanelExecution.ts create mode 100644 packages/frontend/editor-ui/src/features/ai/instanceAi/composables/useSetupPanelTelemetry.ts create mode 100644 packages/frontend/editor-ui/src/features/ai/instanceAi/setupPanelGroups.ts create mode 100644 packages/frontend/editor-ui/src/features/credentials/components/CredentialEdit/CredentialInputs.test.ts diff --git a/packages/@n8n/agents/src/index.ts b/packages/@n8n/agents/src/index.ts index 166b1269788..9daf174a7bd 100644 --- a/packages/@n8n/agents/src/index.ts +++ b/packages/@n8n/agents/src/index.ts @@ -197,6 +197,7 @@ export { renderSkillCatalogPrompt, RUNTIME_SKILL_TOOL_NAMES, RUNTIME_SKILL_FILE_NAME, + RUNTIME_SKILL_MAX_OUTPUT_BYTES, RUNTIME_SKILL_LINKED_FILE_GROUPS, RUNTIME_SKILL_NAME_PATTERN, RUNTIME_SKILL_REGISTRY_SCHEMA_VERSION, diff --git a/packages/@n8n/agents/src/skills/index.ts b/packages/@n8n/agents/src/skills/index.ts index 6bbedc2acb4..3c4c9ebb026 100644 --- a/packages/@n8n/agents/src/skills/index.ts +++ b/packages/@n8n/agents/src/skills/index.ts @@ -1,5 +1,6 @@ export { RUNTIME_SKILL_FILE_NAME, + RUNTIME_SKILL_MAX_OUTPUT_BYTES, RUNTIME_SKILL_LINKED_FILE_GROUPS, RUNTIME_SKILL_REGISTRY_SCHEMA_VERSION, SKILL_LOAD_TOOL_NAME, diff --git a/packages/@n8n/agents/src/skills/tools.ts b/packages/@n8n/agents/src/skills/tools.ts index 7f9cc0d190c..9c2fe780318 100644 --- a/packages/@n8n/agents/src/skills/tools.ts +++ b/packages/@n8n/agents/src/skills/tools.ts @@ -5,6 +5,7 @@ import { Tool } from '../sdk/tool'; import type { BuiltTool } from '../types'; import { RUNTIME_SKILL_FILE_NAME, + RUNTIME_SKILL_MAX_OUTPUT_BYTES, SKILL_LOAD_TOOL_NAME, type RuntimeSkillLinkedFile, type RuntimeSkillLinkedFiles, @@ -15,8 +16,7 @@ import { type RuntimeSkillContent, } from './types'; -const MAX_OUTPUT_BYTES = 64 * 1024; -const TRUNCATION_FOOTER = '\n\n[... output truncated to 64 KB ...]'; +const TRUNCATION_FOOTER = `\n\n[... output truncated to ${RUNTIME_SKILL_MAX_OUTPUT_BYTES / 1024} KB ...]`; const LINKED_FILE_GROUPS: Array = [ 'references', 'templates', @@ -315,6 +315,6 @@ function envelopeValue(value: string): string { function cap(content: string): string { const redacted = scrubSecretsInText(content); const bytes = Buffer.from(redacted, 'utf8'); - if (bytes.byteLength <= MAX_OUTPUT_BYTES) return redacted; - return `${bytes.subarray(0, MAX_OUTPUT_BYTES).toString('utf8')}${TRUNCATION_FOOTER}`; + if (bytes.byteLength <= RUNTIME_SKILL_MAX_OUTPUT_BYTES) return redacted; + return `${bytes.subarray(0, RUNTIME_SKILL_MAX_OUTPUT_BYTES).toString('utf8')}${TRUNCATION_FOOTER}`; } diff --git a/packages/@n8n/agents/src/skills/types.ts b/packages/@n8n/agents/src/skills/types.ts index 2206226ca61..67d87660b13 100644 --- a/packages/@n8n/agents/src/skills/types.ts +++ b/packages/@n8n/agents/src/skills/types.ts @@ -2,6 +2,9 @@ export const RUNTIME_SKILL_REGISTRY_SCHEMA_VERSION = 1 as const; export const RUNTIME_SKILL_FILE_NAME = 'SKILL.md'; +/** Maximum UTF-8 bytes of instruction or linked-file content before truncation. */ +export const RUNTIME_SKILL_MAX_OUTPUT_BYTES = 72 * 1024; + export const SKILL_LOAD_TOOL_NAME = 'load_skill'; export const RUNTIME_SKILL_LINKED_FILE_GROUPS = [ diff --git a/packages/@n8n/api-types/src/schemas/__tests__/agent-run-reducer.test.ts b/packages/@n8n/api-types/src/schemas/__tests__/agent-run-reducer.test.ts index d1e2e72faca..059672feed2 100644 --- a/packages/@n8n/api-types/src/schemas/__tests__/agent-run-reducer.test.ts +++ b/packages/@n8n/api-types/src/schemas/__tests__/agent-run-reducer.test.ts @@ -904,6 +904,11 @@ describe('agent-run-reducer', () => { reduceEvent(state, makeSetupItems('run-1', 'root', 'wf-1', 'slackApi')); reduceEvent(state, makeSetupItems('run-1', 'root', 'wf-1', 'notionApi')); reduceEvent(state, makeSetupItems('run-1', 'root', 'wf-2', 'gmailOAuth2')); + reduceEvent(state, makeSetupItems('run-1', 'root', 'wf-1', 'notionApi')); + expect(state.agentsById['root'].latestSetupAnnouncement).toMatchObject({ + workflowId: 'wf-1', + agentId: 'root', + }); const byWorkflowId = state.agentsById['root'].setupItemsByWorkflowId!; expect(byWorkflowId['wf-1']).toHaveLength(1); @@ -918,6 +923,25 @@ describe('agent-run-reducer', () => { expect(state.agentsById['root'].setupItemsByWorkflowId?.['wf-1']).toHaveLength(1); expect(state.agentsById['sub-1'].setupItemsByWorkflowId).toBeUndefined(); + expect(state.agentsById['root'].latestSetupAnnouncement?.agentId).toBe('sub-1'); + }); + + it('preserves the emitting agent identity across an aliased follow-up run', () => { + const state = stateWithRun('run-1', 'root'); + reduceEvent(state, makeSetupItems('run-1', 'root', 'wf-1', 'slackApi')); + reduceEvent(state, makeRunFinish('run-1', 'root', 'completed')); + reduceEvent(state, makeRunStart('run-2', 'follow-up-root')); + reduceEvent(state, makeToolCall('run-2', 'follow-up-root', 'build-2', 'build-workflow')); + reduceEvent(state, makeSetupItems('run-2', 'follow-up-root', 'wf-2', 'notionApi')); + + const root = toAgentTree(state); + expect(root.latestSetupAnnouncement).toMatchObject({ + workflowId: 'wf-2', + agentId: root.agentId, + }); + expect(root.toolCalls).toContainEqual( + expect.objectContaining({ toolCallId: 'build-2', isLoading: true }), + ); }); it('survives a tree snapshot round trip (history restore)', () => { @@ -932,6 +956,9 @@ describe('agent-run-reducer', () => { expect(restored?.agentsById['root'].setupItemsByWorkflowId?.['wf-1'][0]).toMatchObject({ credentialType: 'slackApi', }); + expect(restored?.agentsById['root'].latestSetupAnnouncement).toEqual( + state.agentsById['root'].latestSetupAnnouncement, + ); }); it('is preserved across a follow-up run-start when it is the only content', () => { diff --git a/packages/@n8n/api-types/src/schemas/agent-run-reducer.ts b/packages/@n8n/api-types/src/schemas/agent-run-reducer.ts index caaa81905bb..eef07d49e82 100644 --- a/packages/@n8n/api-types/src/schemas/agent-run-reducer.ts +++ b/packages/@n8n/api-types/src/schemas/agent-run-reducer.ts @@ -499,6 +499,11 @@ export function reduceEvent(state: AgentRunState, event: InstanceAiEvent): Agent // semantics: last event wins per workflowId. const root = ensureAgent(state, state.rootAgentId); if (root && isSafeObjectKey(event.payload.workflowId)) { + root.latestSetupAnnouncement = { + workflowId: event.payload.workflowId, + agentId: ensureAgent(state, event.agentId)?.agentId ?? event.agentId, + timestamp: eventTimestamp(event), + }; root.setupItemsByWorkflowId = { ...root.setupItemsByWorkflowId, [event.payload.workflowId]: event.payload.items, diff --git a/packages/@n8n/api-types/src/schemas/instance-ai.schema.ts b/packages/@n8n/api-types/src/schemas/instance-ai.schema.ts index 47fce92b355..c83bc0ce745 100644 --- a/packages/@n8n/api-types/src/schemas/instance-ai.schema.ts +++ b/packages/@n8n/api-types/src/schemas/instance-ai.schema.ts @@ -1795,6 +1795,8 @@ export interface InstanceAiAgentNode { * regardless of which agent emitted. */ setupItemsByWorkflowId?: Record; + /** Latest setup announcement, including its emitting agent and replay-stable time. */ + latestSetupAnnouncement?: { workflowId: string; agentId: string; timestamp: string }; result?: string; error?: string; errorDetails?: { diff --git a/packages/@n8n/instance-ai/skills/post-build-flow/SKILL.md b/packages/@n8n/instance-ai/skills/post-build-flow/SKILL.md index 201ffcd7b17..fb71d3319ff 100644 --- a/packages/@n8n/instance-ai/skills/post-build-flow/SKILL.md +++ b/packages/@n8n/instance-ai/skills/post-build-flow/SKILL.md @@ -39,6 +39,9 @@ Use this section when the system prompt describes the persistent setup panel, setup returns `announced: true`, or the current user input contains ``. Otherwise, keep the setup card flow below. +- Setup requirements can appear while the workflow is being built. The user can + complete them immediately. Refer to the "setup panel" without a position or + a claim that setup must wait until the build finishes. - Verify what the build can simulate before asking the user to finish setup. Missing credentials do not prevent this verification. Report which outputs were simulated. A simulated result does not prove a live connection. diff --git a/packages/@n8n/instance-ai/skills/workflow-builder/SKILL.md b/packages/@n8n/instance-ai/skills/workflow-builder/SKILL.md index 859b2f1b5b7..ddfb51f48c6 100644 --- a/packages/@n8n/instance-ai/skills/workflow-builder/SKILL.md +++ b/packages/@n8n/instance-ai/skills/workflow-builder/SKILL.md @@ -516,7 +516,9 @@ When `nodes(action="explore-resources")` returns no results for a required resource: 1. If the resource can be represented as a user choice, use - `placeholder('Select ')` and let setup collect it after the build. + `placeholder('Select ')` and let setup collect it. When the persistent + setup panel is enabled, the user can fill announced requirements during the + build. Do not tell them to wait until the build finishes. 2. If the user explicitly asked you to create the resource and the node type definition has a safe create operation, build and verify that resource-creation workflow as part of the requested work. diff --git a/packages/@n8n/instance-ai/src/agent/__tests__/system-prompt.setup-panel.test.ts b/packages/@n8n/instance-ai/src/agent/__tests__/system-prompt.setup-panel.test.ts index 5b6649e2201..c03670f508a 100644 --- a/packages/@n8n/instance-ai/src/agent/__tests__/system-prompt.setup-panel.test.ts +++ b/packages/@n8n/instance-ai/src/agent/__tests__/system-prompt.setup-panel.test.ts @@ -18,7 +18,8 @@ describe('getSystemPrompt — setup panel', () => { const prompt = getSystemPrompt({ setupPanelEnabled: true }); expect(prompt).not.toContain('opens the inline setup card'); - expect(prompt).toContain('setup panel next to the chat'); + expect(prompt).toContain('setup panel while the workflow is being built'); + expect(prompt).not.toContain('next to the chat'); expect(prompt).toContain(''); expect(prompt).toContain('end your turn'); expect(prompt).toContain('When the result has `announced: true`'); diff --git a/packages/@n8n/instance-ai/src/agent/system-prompt.ts b/packages/@n8n/instance-ai/src/agent/system-prompt.ts index f8b8b0fd669..fd89d4c7fab 100644 --- a/packages/@n8n/instance-ai/src/agent/system-prompt.ts +++ b/packages/@n8n/instance-ai/src/agent/system-prompt.ts @@ -210,7 +210,7 @@ If the user asks for a blocked operation, explain that the instance is in read-o */ function getCredentialSetupBullet(setupPanelEnabled?: boolean): string { if (setupPanelEnabled) { - return '**Credential setup** uses `workflows(action="setup")` when a workflowId is available. When the result has `announced: true`, the setup panel next to the chat lists the remaining credentials and parameters. Summarize that result, report any validation warnings, and end your turn. Other results need their returned guidance: correct validation errors, respect denials and skipped items, and wait for requested destination approvals. Explicit credential replacement and an already-open setup card keep their card flow, including apply and test-trigger results. Do not treat a resumed card as a panel announcement. Each new user turn carries a `` block with current configuration; trust it over older tool results. Configuration alone does not prove successful testing. Use `credentials(action="setup")` when the user explicitly asks to create a credential outside of any workflow context. Never call both tools for the same workflow. Never describe workflow setup as something the user starts from the canvas or editor, and never ask the user to paste secrets into chat.'; + return '**Credential setup** uses `workflows(action="setup")` when a workflowId is available. Requirements can appear in the setup panel while the workflow is being built. The user can complete them immediately. Do not describe setup as happening only after the build. When the result has `announced: true`, the setup panel lists the remaining credentials and parameters. Summarize that result, report any validation warnings, and end your turn. Other results need their returned guidance: correct validation errors, respect denials and skipped items, and wait for requested destination approvals. Explicit credential replacement and an already-open setup card keep their card flow, including apply and test-trigger results. Do not treat a resumed card as a panel announcement. Each new user turn carries a `` block with current configuration; trust it over older tool results. Configuration alone does not prove successful testing. Use `credentials(action="setup")` when the user explicitly asks to create a credential outside of any workflow context. Never call both tools for the same workflow. Never describe workflow setup as something the user starts from the canvas or editor, and never ask the user to paste secrets into chat.'; } return '**Credential setup** uses `workflows(action="setup")` when a workflowId is available — it opens the inline setup card in the n8n Assistant panel and handles credentials, parameters, and triggers in one step. Use `credentials(action="setup")` only when the user explicitly asks to create a credential outside of any workflow context. Never call both tools for the same workflow. Never describe workflow setup as something the user starts from the canvas or editor. Setup cards are only open while the setup call is pending — once it returns a result, the card is resolved: describe the outcome (e.g. credentials selected and ready), never that a card is open or that the user still needs to authorize. When a node in `nodesStillNeedingSetup` carries `parameterIssues`, the connected credential can\'t reach the value that was configured (e.g. a model outside what the credential allows) — fix the value, then tell the user plainly which value didn\'t work and what you set instead. Never silently swap a model or other parameter without saying so. Nodes listed under `skippedByUser` are different: the user chose to skip them, so never re-open the setup card for those — say what stays unconfigured and offer to set it up later.'; } diff --git a/packages/@n8n/instance-ai/src/skills/__tests__/materialize-runtime-skills.test.ts b/packages/@n8n/instance-ai/src/skills/__tests__/materialize-runtime-skills.test.ts index 1441bff6e0d..051bcf3a6e8 100644 --- a/packages/@n8n/instance-ai/src/skills/__tests__/materialize-runtime-skills.test.ts +++ b/packages/@n8n/instance-ai/src/skills/__tests__/materialize-runtime-skills.test.ts @@ -1,4 +1,5 @@ import { + RUNTIME_SKILL_MAX_OUTPUT_BYTES, RUNTIME_SKILL_REGISTRY_SCHEMA_VERSION, createSkillLoadTool, type RuntimeSkillLinkedFiles, @@ -356,7 +357,6 @@ describe('materializeRuntimeSkillsIntoWorkspace', () => { }); it('warns when materialized skill files exceed the load_skill output limit', async () => { - const runtimeSkillMaxOutputBytes = 64 * 1024; const source: RuntimeSkillSource = { registry: { schemaVersion: RUNTIME_SKILL_REGISTRY_SCHEMA_VERSION, @@ -376,7 +376,7 @@ describe('materializeRuntimeSkillsIntoWorkspace', () => { id: 'large-skill', name: 'large-skill', description: 'Large skill', - instructions: 'x'.repeat(runtimeSkillMaxOutputBytes + 1), + instructions: 'x'.repeat(RUNTIME_SKILL_MAX_OUTPUT_BYTES + 1), }), }; const { workspace } = createMockWorkspace(); @@ -399,7 +399,7 @@ describe('materializeRuntimeSkillsIntoWorkspace', () => { const [message, meta] = limitWarnCall!; expect(message).toBe('Runtime skill file exceeds load_skill output limit'); expect(meta?.skill).toBe('large-skill'); - expect(meta?.maxBytes).toBe(runtimeSkillMaxOutputBytes); + expect(meta?.maxBytes).toBe(RUNTIME_SKILL_MAX_OUTPUT_BYTES); }); }); diff --git a/packages/@n8n/instance-ai/src/skills/materialize-runtime-skills.ts b/packages/@n8n/instance-ai/src/skills/materialize-runtime-skills.ts index c24db21a81d..fb10bdce8b2 100644 --- a/packages/@n8n/instance-ai/src/skills/materialize-runtime-skills.ts +++ b/packages/@n8n/instance-ai/src/skills/materialize-runtime-skills.ts @@ -1,5 +1,6 @@ import { RUNTIME_SKILL_FILE_NAME, + RUNTIME_SKILL_MAX_OUTPUT_BYTES, type RuntimeSkillContent, type RuntimeSkillDependenciesContract, type RuntimeSkillInterfaceContract, @@ -91,7 +92,6 @@ const LINKED_FILE_GROUPS = [ const N8N_SKILL_DIR_TEMPLATE = '$' + '{N8N_SKILL_DIR}'; const N8N_SKILLS_DIR_TEMPLATE = '$' + '{N8N_SKILLS_DIR}'; const N8N_WORKSPACE_DIR_TEMPLATE = '$' + '{N8N_WORKSPACE_DIR}'; -const LOAD_SKILL_OUTPUT_LIMIT_BYTES = 64 * 1024; function isNonEmptyRecord(value: Record): boolean { return Object.keys(value).length > 0; @@ -410,13 +410,13 @@ function warnIfExceedsLoadSkillLimit( content: string, ): void { const bytes = Buffer.byteLength(content, 'utf8'); - if (bytes <= LOAD_SKILL_OUTPUT_LIMIT_BYTES) return; + if (bytes <= RUNTIME_SKILL_MAX_OUTPUT_BYTES) return; logger.warn('Runtime skill file exceeds load_skill output limit', { skill: entry.name, path: filePath, bytes, - maxBytes: LOAD_SKILL_OUTPUT_LIMIT_BYTES, + maxBytes: RUNTIME_SKILL_MAX_OUTPUT_BYTES, }); } diff --git a/packages/@n8n/instance-ai/src/tools/credentials.tool.ts b/packages/@n8n/instance-ai/src/tools/credentials.tool.ts index a535b1d7977..3b85581af98 100644 --- a/packages/@n8n/instance-ai/src/tools/credentials.tool.ts +++ b/packages/@n8n/instance-ai/src/tools/credentials.tool.ts @@ -804,7 +804,7 @@ async function announceSetupItems( workflowId, credentials: existingByType, message: - `Listed ${typeNames} in the setup panel next to the chat. No card is open and nothing is waiting on ` + + `Listed ${typeNames} in the setup panel. No card is open and nothing is waiting on ` + 'you: continue the build. The user can connect these at any time while you work — the build ' + 'attaches a matching stored credential automatically and the panel tracks what remains. Do not ' + 'ask the user to connect them now, do not describe a card, and do not call setup again for these ' + diff --git a/packages/@n8n/instance-ai/src/tools/workflows.tool.ts b/packages/@n8n/instance-ai/src/tools/workflows.tool.ts index aac433a4d47..a9695fa1f03 100644 --- a/packages/@n8n/instance-ai/src/tools/workflows.tool.ts +++ b/packages/@n8n/instance-ai/src/tools/workflows.tool.ts @@ -1493,7 +1493,7 @@ async function resolveUnverifiedPublishDisclosure( } const SETUP_PANEL_ANNOUNCED_GUIDANCE = - 'The setup panel next to the chat now lists what this workflow still needs (`open`); nothing is ' + + 'The setup panel now lists what this workflow still needs (`open`); nothing is ' + 'waiting on you and no card is open. Finish your turn now: tell the user in one or two sentences ' + 'what to configure in the panel — name the services and any values — then stop. Do not call setup ' + 'again for this workflow, do not call `credentials(action="setup")`, and do not tell the user to ' + diff --git a/packages/@n8n/instance-ai/src/tools/workflows/setup-panel-state.ts b/packages/@n8n/instance-ai/src/tools/workflows/setup-panel-state.ts index 140a6a1ef3b..42c07171a8e 100644 --- a/packages/@n8n/instance-ai/src/tools/workflows/setup-panel-state.ts +++ b/packages/@n8n/instance-ai/src/tools/workflows/setup-panel-state.ts @@ -270,7 +270,7 @@ export function formatWorkflowSetupStateNote( })); return [ 'Setup state of the workflows this conversation built, recomputed just now from the saved ' + - 'workflows and the credentials in this project. The setup panel next to the chat shows the ' + + 'workflows and the credentials in this project. The setup panel shows the ' + 'current checklist and the user completes items there. This observation does not test connections ' + 'or resource availability. A configured item is not proof of a successful connection test or ' + 'workflow execution. Never ask ' + diff --git a/packages/@n8n/instance-ai/src/workflow-loop/__tests__/guidance.test.ts b/packages/@n8n/instance-ai/src/workflow-loop/__tests__/guidance.test.ts index 70770bfb9a7..d9154a07aa7 100644 --- a/packages/@n8n/instance-ai/src/workflow-loop/__tests__/guidance.test.ts +++ b/packages/@n8n/instance-ai/src/workflow-loop/__tests__/guidance.test.ts @@ -553,7 +553,7 @@ describe('formatWorkflowLoopGuidance — setup panel', () => { expect(result).toContain('workflows(action="setup")'); expect(result).toContain('wf-123'); - expect(result).toContain('setup panel next to the chat'); + expect(result).toContain('setup panel'); expect(result).toContain('end your turn'); expect(result).toContain('When the result has `announced: true`'); expect(result).toContain('Otherwise follow the returned guidance'); diff --git a/packages/@n8n/instance-ai/src/workflow-loop/guidance.ts b/packages/@n8n/instance-ai/src/workflow-loop/guidance.ts index 1a80900ddf3..ad63bea0e18 100644 --- a/packages/@n8n/instance-ai/src/workflow-loop/guidance.ts +++ b/packages/@n8n/instance-ai/src/workflow-loop/guidance.ts @@ -97,7 +97,7 @@ export function formatWorkflowLoopGuidance( return ( `${claimLead} It still uses temporary mock data. ` + `Call \`workflows(action="setup")\` with workflowId "${action.workflowId ?? 'unknown'}" once: ` + - 'it lists the remaining credentials and values in the setup panel next to the chat and returns them to you. ' + + 'it lists the remaining credentials and values in the setup panel and returns them to you. ' + 'When the result has `announced: true`, summarize it, report any validation warnings, and end your turn. ' + 'Otherwise follow the returned guidance for validation errors, approvals, skipped items, or an existing setup card. ' + 'Do not call `credentials(action="setup")` or `apply-workflow-credentials`, and do not tell the user to open the editor or canvas.' diff --git a/packages/@n8n/telemetry/src/events/credentials.ts b/packages/@n8n/telemetry/src/events/credentials.ts index c3dd86c75ab..458fa0d606d 100644 --- a/packages/@n8n/telemetry/src/events/credentials.ts +++ b/packages/@n8n/telemetry/src/events/credentials.ts @@ -3,6 +3,32 @@ import { z } from 'zod/v4'; import { defineTelemetryEvents } from '../define'; export const CREDENTIALS_TELEMETRY = defineTelemetryEvents({ + USER_STARTED_CREDENTIAL_CONNECTION: { + name: 'User started credential connection', + description: + 'The user started a connection attempt or selected an existing credential in the setup panel. Includes retries.', + properties: z.object({ + source: z.literal('instance_ai_setup_panel'), + workflow_id: z.string(), + thread_id: z.string(), + credential_type: z.string(), + method: z.enum(['oauth', 'api_key', 'gateway', 'advanced', 'existing']), + }), + }, + USER_COMPLETED_CREDENTIAL_CONNECTION: { + name: 'User completed credential connection', + description: + 'A setup-panel credential was selected or connected and its workflow binding was applied or queued. This does not report workflow validation.', + properties: z.object({ + source: z.literal('instance_ai_setup_panel'), + workflow_id: z.string(), + thread_id: z.string(), + credential_type: z.string(), + credential_id: z.string().nullable(), + method: z.enum(['oauth', 'api_key', 'gateway', 'advanced', 'existing']), + binding_state: z.enum(['applied', 'noop', 'queued']), + }), + }, USER_PROBED_CREDENTIAL: { name: 'User probed credential', description: diff --git a/packages/@n8n/telemetry/src/events/instance-ai.ts b/packages/@n8n/telemetry/src/events/instance-ai.ts index 042f7640d63..20bb3a203c5 100644 --- a/packages/@n8n/telemetry/src/events/instance-ai.ts +++ b/packages/@n8n/telemetry/src/events/instance-ai.ts @@ -52,6 +52,47 @@ const threadActionSource = z.enum([ const prefillType = z.enum([...INSTANCE_AI_PREFILL_TYPES, INSTANCE_AI_PREFILL_TYPE_FALLBACK]); export const INSTANCE_AI_TELEMETRY = defineTelemetryEvents({ + SETUP_PANEL_STATE_OBSERVED: { + name: 'AI Assistant setup panel state observed', + description: + 'The setup panel observed a new requirement snapshot. Also fires when no setup is needed, including workflows whose credentials were already connected.', + properties: z.object({ + workflow_id: z.string(), + thread_id: z.string(), + credential_count: z.number(), + pending_credential_count: z.number(), + pending_parameter_count: z.number(), + already_connected_count: z.number(), + }), + }, + SETUP_PANEL_ITEM_SHOWN: { + name: 'AI Assistant setup panel item shown', + description: + 'A setup checklist row became visible. Reopening the same row within the mounted panel does not emit another event.', + properties: z.object({ + workflow_id: z.string(), + thread_id: z.string(), + kind: z.enum(['credential', 'parameters', 'details']), + credential_type: z.string().optional(), + parameter_count: z.number(), + }), + }, + SETUP_PANEL_DISMISSED: { + name: 'AI Assistant setup panel dismissed', + description: + 'A visible setup panel closed. The reason separates navigation and removed requirements from a finished execution or an explicit dismissal.', + properties: z.object({ + workflow_id: z.string(), + thread_id: z.string(), + reason: z.enum([ + 'navigation', + 'items_removed', + 'execution_succeeded', + 'execution_finished', + 'user_dismissed', + ]), + }), + }, USER_CLICKED_AI_CREDIT_BALANCE: { name: 'User clicked AI credit balance', description: diff --git a/packages/@n8n/telemetry/src/events/workflow.ts b/packages/@n8n/telemetry/src/events/workflow.ts index 2ffcd6ff074..3216b9c954b 100644 --- a/packages/@n8n/telemetry/src/events/workflow.ts +++ b/packages/@n8n/telemetry/src/events/workflow.ts @@ -3,6 +3,16 @@ import { z } from 'zod/v4'; import { defineTelemetryEvents } from '../define'; export const WORKFLOW_TELEMETRY = defineTelemetryEvents({ + USER_REQUESTED_WORKFLOW_TEST: { + name: 'User requested workflow test', + description: + 'The user started a manual workflow test from the AI Assistant setup panel. This event does not report execution success.', + properties: z.object({ + source: z.literal('instance_ai_setup_panel'), + workflow_id: z.string(), + thread_id: z.string().optional(), + }), + }, MULTIPLE_NODES_SELECTED: { name: 'User selected multiple nodes', description: diff --git a/packages/frontend/@n8n/design-system/src/components/N8nSetupConnection/SetupConnection.stories.ts b/packages/frontend/@n8n/design-system/src/components/N8nSetupConnection/SetupConnection.stories.ts new file mode 100644 index 00000000000..e2430753467 --- /dev/null +++ b/packages/frontend/@n8n/design-system/src/components/N8nSetupConnection/SetupConnection.stories.ts @@ -0,0 +1,29 @@ +import type { Meta, StoryObj } from '@storybook/vue3-vite'; + +import N8nSetupConnection from './SetupConnection.vue'; + +const meta = { + title: 'Areas/Assistant/SetupConnection', + component: N8nSetupConnection, + args: { + connected: false, + actionLabel: 'Connect', + actions: [{ id: 'advanced', label: 'Advanced setup' }], + }, +} satisfies Meta; +export default meta; +type Story = StoryObj; + +export const Connect: Story = {}; +export const Connecting: Story = { args: { loading: true } }; +export const Connected: Story = { + args: { + connected: true, + valueLabel: 'Account', + value: 'demo@example.com', + actions: [ + { id: 'switch', label: 'Switch account' }, + { id: 'edit', label: 'Edit credential' }, + ], + }, +}; diff --git a/packages/frontend/@n8n/design-system/src/components/N8nSetupConnection/SetupConnection.test.ts b/packages/frontend/@n8n/design-system/src/components/N8nSetupConnection/SetupConnection.test.ts new file mode 100644 index 00000000000..b672ce5c275 --- /dev/null +++ b/packages/frontend/@n8n/design-system/src/components/N8nSetupConnection/SetupConnection.test.ts @@ -0,0 +1,62 @@ +import userEvent from '@testing-library/user-event'; +import { render } from '@testing-library/vue'; + +import SetupConnection from './SetupConnection.vue'; + +describe('SetupConnection', () => { + it('keeps Advanced setup available while required fields disable the primary action', async () => { + const { getByRole, getAllByRole, findByRole, emitted, rerender } = render(SetupConnection, { + props: { + connected: false, + actionLabel: 'Connect', + actionDisabled: true, + actions: [{ id: 'advanced', label: 'Advanced setup' }], + }, + }); + expect(getByRole('button', { name: 'Connect' })).toBeDisabled(); + expect(getAllByRole('button')).toEqual([ + getByRole('button', { name: 'More options' }), + getByRole('button', { name: 'Connect' }), + ]); + await userEvent.click(getByRole('button', { name: 'More options' })); + await userEvent.click(await findByRole('menuitem', { name: 'Advanced setup' })); + expect(emitted('select')).toEqual([['advanced']]); + expect(emitted('action')).toBeUndefined(); + await rerender({ actionDisabled: false }); + await userEvent.click(getByRole('button', { name: 'Connect' })); + expect(emitted('action')).toHaveLength(1); + }); + + it('shows a saved connection and opens its pencil menu without starting a connection', async () => { + const { getByRole, getByText, findByRole, queryByRole, emitted } = render(SetupConnection, { + props: { + connected: true, + value: 'user@example.com', + valueLabel: 'Account', + actionLabel: 'Connect', + actions: [{ id: 'replace', label: 'Switch account' }], + }, + }); + expect(getByText('user@example.com')).toBeVisible(); + expect(queryByRole('button', { name: 'Connect' })).toBeNull(); + await userEvent.click(getByRole('button', { name: 'Change connection' })); + await userEvent.click(await findByRole('menuitem', { name: 'Switch account' })); + expect(emitted('select')).toEqual([['replace']]); + expect(emitted('action')).toBeUndefined(); + }); + + it.each([false, true])( + 'disables connection controls during a request, connected: %s', + (connected) => { + const { getAllByRole } = render(SetupConnection, { + props: { + connected, + loading: true, + actionLabel: 'Connect', + actions: [{ id: 'edit', label: 'Edit' }], + }, + }); + for (const button of getAllByRole('button')) expect(button).toBeDisabled(); + }, + ); +}); diff --git a/packages/frontend/@n8n/design-system/src/components/N8nSetupConnection/SetupConnection.types.ts b/packages/frontend/@n8n/design-system/src/components/N8nSetupConnection/SetupConnection.types.ts new file mode 100644 index 00000000000..16af8a4ecf8 --- /dev/null +++ b/packages/frontend/@n8n/design-system/src/components/N8nSetupConnection/SetupConnection.types.ts @@ -0,0 +1,23 @@ +import type { ButtonProps } from '../../types/button'; +import type { DropdownMenuItemProps } from '../N8nDropdownMenu/DropdownMenu.types'; + +export interface SetupConnectionProps { + /** Whether the host has a saved connection to display. */ + connected: boolean; + /** Label for the saved account or authentication value. */ + valueLabel?: string; + /** Public account identifier, credential name, or masked key. Never pass a secret. */ + value?: string; + /** Label for the primary connect or save action. */ + actionLabel: string; + /** Button treatment for the connect or save action. */ + actionVariant?: ButtonProps['variant']; + /** Secondary actions for the current connection method. */ + actions: DropdownMenuItemProps[]; + /** Whether the primary action is unavailable, such as for an incomplete form. */ + actionDisabled?: boolean; + /** Whether a connection attempt or save is in progress. */ + loading?: boolean; + /** Whether the connection and its secondary actions are unavailable. */ + disabled?: boolean; +} diff --git a/packages/frontend/@n8n/design-system/src/components/N8nSetupConnection/SetupConnection.vue b/packages/frontend/@n8n/design-system/src/components/N8nSetupConnection/SetupConnection.vue new file mode 100644 index 00000000000..dd20c63d946 --- /dev/null +++ b/packages/frontend/@n8n/design-system/src/components/N8nSetupConnection/SetupConnection.vue @@ -0,0 +1,153 @@ + + + + + diff --git a/packages/frontend/@n8n/design-system/src/components/N8nSetupPanel/SetupPanel.stories.ts b/packages/frontend/@n8n/design-system/src/components/N8nSetupPanel/SetupPanel.stories.ts new file mode 100644 index 00000000000..ae56454a219 --- /dev/null +++ b/packages/frontend/@n8n/design-system/src/components/N8nSetupPanel/SetupPanel.stories.ts @@ -0,0 +1,356 @@ +import type { Meta, StoryObj } from '@storybook/vue3-vite'; +import { usePreferredReducedMotion, useTimeoutFn } from '@vueuse/core'; +import { computed, ref, watch } from 'vue'; + +import N8nSelect from '../../v2/components/Select/Select.vue'; +import N8nButton from '../N8nButton'; +import N8nCopyInput from '../N8nCopyInput'; +import N8nIcon from '../N8nIcon'; +import N8nInput from '../N8nInput'; +import N8nSegmentControl from '../N8nSegmentControl'; +import N8nText from '../N8nText'; +import N8nSetupPanel from './SetupPanel.vue'; +import N8nSetupConnection from '../N8nSetupConnection/SetupConnection.vue'; + +const meta = { + title: 'Areas/Assistant/SetupPanel', + component: N8nSetupPanel, + parameters: { layout: 'centered' }, +} satisfies Meta; +export default meta; +type Story = StoryObj; + +export const Checklist: Story = { + args: { + items: [ + { id: 'granola', title: 'Granola', completed: true }, + { id: 'slack', title: 'Slack', subtitle: 'Add connection, channel', completed: false }, + { id: 'details', title: 'Details', subtitle: 'Add summary length', completed: false }, + ], + }, + render: (args) => ({ + components: { N8nSetupPanel, N8nIcon, N8nInput }, + setup: () => ({ args, active: ref() }), + template: ` +
+ + + + + +
+ `, + }), +}; + +export const SetupComplete: Story = { + ...Checklist, + args: { + items: [ + { id: 'slack', title: 'Slack', completed: true }, + { id: 'details', title: 'Details', completed: true }, + ], + status: 'complete', + }, +}; + +export const Validating: Story = { + ...SetupComplete, + args: { ...SetupComplete.args, status: 'validating' }, +}; +export const Executing: Story = { + ...SetupComplete, + args: { ...SetupComplete.args, status: 'executing' }, +}; +export const ExecuteUnavailable: Story = { + ...SetupComplete, + args: { ...SetupComplete.args, executeDisabled: true }, +}; + +type Example = { + service: string; + method: 'oauth' | 'key' | 'selfHosted' | 'gateway' | 'details'; + connected?: boolean; + confirmed?: boolean; + field?: string; + custom?: boolean; + useOwnKey?: boolean; + /** Show the full completion flow instead of a static state example. */ + interactive?: boolean; +}; + +function example(state: Example): Story { + return { + args: { items: [] }, + render: () => ({ + components: { + N8nSetupPanel, + N8nSetupConnection, + N8nButton, + N8nCopyInput, + N8nInput, + N8nIcon, + N8nSegmentControl, + N8nText, + N8nSelect, + }, + setup() { + const active = ref( + state.method === 'oauth' && !state.connected ? undefined : 'service', + ); + const connected = ref(state.connected ?? false); + const confirmed = ref(state.confirmed ?? false); + const mode = ref(state.useOwnKey ? 'key' : 'credits'); + const key = ref(''); + const clientId = ref(''); + const clientSecret = ref(''); + const fieldValue = ref('suggested'); + const savedField = ref(state.confirmed ? 'suggested' : undefined); + const menuFeedback = ref(''); + const completed = computed( + () => + (state.method === 'details' || connected.value) && (!state.field || confirmed.value), + ); + const reducedMotion = usePreferredReducedMotion(); + const { start: returnToChecklist, stop: cancelReturn } = useTimeoutFn( + () => { + active.value = undefined; + }, + computed(() => (reducedMotion.value === 'reduce' ? 0 : 650)), + { immediate: false }, + ); + watch([active, fieldValue], cancelReturn); + function confirmField() { + confirmed.value = true; + savedField.value = fieldValue.value; + if (state.interactive) returnToChecklist(); + } + const items = computed(() => [ + { + id: 'service', + title: state.service, + completed: completed.value, + hasAction: state.method === 'oauth' && !connected.value, + subtitle: `Add ${!connected.value && state.method !== 'details' ? 'connection' : state.field}`, + }, + ]); + const actionLabel = computed(() => { + if (state.method === 'gateway') return mode.value === 'credits' ? 'Use credits' : 'Save'; + return state.method === 'selfHosted' + ? 'Save and sign in' + : state.method === 'key' + ? 'Save' + : 'Connect'; + }); + const actions = computed(() => { + if (!connected.value && state.method === 'gateway' && mode.value === 'credits') return []; + if (!connected.value) return [{ id: 'advanced', label: 'Advanced setup' }]; + if (state.method === 'gateway' && mode.value === 'credits') + return [ + { id: 'replace', label: 'Use my API key' }, + { id: 'manage', label: 'Manage Gateway credits' }, + ]; + if (state.custom) + return [ + { id: 'edit', label: 'Edit credential' }, + { id: 'switch', label: 'Connect with OAuth instead' }, + ]; + return [ + { + id: 'replace', + label: + state.method === 'key' || state.method === 'gateway' + ? 'Create new credential' + : 'Switch account', + }, + { id: 'edit', label: 'Edit credential' }, + ]; + }); + const value = computed(() => { + if (state.method === 'gateway' && mode.value === 'credits') return 'Gateway credits'; + if (state.custom || state.method === 'key' || state.method === 'gateway') + return '••••••••'; + return 'demo@example.com'; + }); + return { + state, + menuFeedback, + confirmField, + status: computed(() => + state.interactive + ? completed.value && savedField.value === fieldValue.value + ? 'complete' + : 'incomplete' + : undefined, + ), + active, + connected, + confirmed, + mode, + key, + clientId, + clientSecret, + fieldValue, + savedField, + items, + actions, + actionLabel, + value, + valueLabel: state.method === 'key' || state.method === 'gateway' ? 'API key' : 'Account', + actionDisabled: computed(() => + state.method === 'selfHosted' + ? !clientId.value || !clientSecret.value + : (state.method === 'key' || (state.method === 'gateway' && mode.value === 'key')) && + !key.value, + ), + onMenuAction: (id: string) => { + menuFeedback.value = actions.value.find((action) => action.id === id)?.label ?? ''; + if (id === 'replace' || id === 'switch') { + connected.value = false; + mode.value = 'key'; + } + }, + modeOptions: [ + { value: 'credits', label: 'Gateway credits' }, + { value: 'key', label: 'Use my API key' }, + ], + fieldOptions: [ + { + value: 'suggested', + label: state.field === 'channel' ? '#design-team' : 'Suggested value', + }, + { value: 'other', label: 'Another value' }, + ], + }; + }, + template: ` +
+ + + + + + +
+ `, + }), + }; +} + +export const ManagedOAuth = example({ service: 'Granola', method: 'oauth' }); +export const ManagedOAuthConnected = example({ + service: 'Granola', + method: 'oauth', + connected: true, +}); +export const AdvancedSetupConnected = example({ + service: 'Granola', + method: 'oauth', + connected: true, + custom: true, +}); +export const ManagedOAuthWithDetails = example({ + service: 'Slack', + method: 'oauth', + field: 'channel', +}); +export const ConnectedWithDetailsPending = example({ + service: 'Slack', + method: 'oauth', + connected: true, + field: 'channel', +}); +export const ServiceComplete = example({ + service: 'Slack', + method: 'oauth', + connected: true, + field: 'channel', + confirmed: true, +}); +export const ApiKey = example({ service: 'Clearbit', method: 'key' }); +export const ApiKeyConnected = example({ service: 'Clearbit', method: 'key', connected: true }); +export const GatewayCredits = example({ service: 'OpenAI', method: 'gateway' }); +export const StateTransitions = example({ + service: 'Example service', + method: 'gateway', + field: 'Notification channel', + interactive: true, +}); +export const GatewayOwnKey = example({ service: 'OpenAI', method: 'gateway', useOwnKey: true }); +export const GatewayCreditsConnected = example({ + service: 'OpenAI', + method: 'gateway', + connected: true, +}); +export const GatewayOwnKeyConnected = example({ + service: 'OpenAI', + method: 'gateway', + connected: true, + useOwnKey: true, +}); +export const SelfHostedOAuth = example({ + service: 'LinkedIn', + method: 'selfHosted', + field: 'Post as', +}); +export const SelfHostedDetailsPending = example({ + service: 'LinkedIn', + method: 'selfHosted', + connected: true, + field: 'Post as', +}); +export const SelfHostedComplete = example({ + service: 'LinkedIn', + method: 'selfHosted', + connected: true, + field: 'Post as', + confirmed: true, +}); +export const Details = example({ service: 'Details', method: 'details', field: 'Summary length' }); +export const DetailsComplete = example({ + service: 'Details', + method: 'details', + field: 'Summary length', + confirmed: true, +}); diff --git a/packages/frontend/@n8n/design-system/src/components/N8nSetupPanel/SetupPanel.test.ts b/packages/frontend/@n8n/design-system/src/components/N8nSetupPanel/SetupPanel.test.ts new file mode 100644 index 00000000000..08c27087ff2 --- /dev/null +++ b/packages/frontend/@n8n/design-system/src/components/N8nSetupPanel/SetupPanel.test.ts @@ -0,0 +1,239 @@ +import userEvent from '@testing-library/user-event'; +import { fireEvent, render, waitFor } from '@testing-library/vue'; +import { nextTick, ref } from 'vue'; + +import SetupPanel from './SetupPanel.vue'; + +describe('SetupPanel', () => { + it('replaces the service icon on completion without showing a progress count', async () => { + const items = [ + { id: 'slack', title: 'Slack', completed: false }, + { id: 'gmail', title: 'Gmail', completed: true }, + ]; + const { getByRole, queryByText, rerender } = render(SetupPanel, { + props: { items, activeItemId: 'slack' }, + slots: { icon: '' }, + }); + const detail = getByRole('dialog', { name: 'Slack' }); + expect(detail.querySelector('[data-test-id="service-icon"]')).toBeVisible(); + await rerender({ items: items.map((item) => ({ ...item, completed: true })) }); + expect(detail.querySelector('[data-test-id="service-icon"]')).toBeNull(); + expect(detail.querySelector('[data-icon="status-completed"]')).toBeVisible(); + await rerender({ activeItemId: undefined, status: 'complete' }); + expect(getByRole('button', { name: 'Setup complete' })).toBeVisible(); + expect(getByRole('button', { name: 'Execute' })).toBeVisible(); + expect(queryByText(/\d+\s*(?:of|\/)\s*\d+/i)).toBeNull(); + }); + + it('retains outgoing content while making the closing detail inaccessible', async () => { + const activeItemId = ref('slack'); + const { getByRole, getByTestId, queryByRole, unmount } = render( + { + components: { SetupPanel }, + setup: () => ({ activeItemId, items: [{ id: 'slack', title: 'Slack', completed: false }] }), + template: ` + + `, + }, + { global: { stubs: { transition: false } } }, + ); + const overlay = getByTestId('setup-panel-overlay'); + overlay.style.transitionDuration = '1s'; + overlay.style.transitionDelay = '0s'; + await fireEvent.click(getByRole('button', { name: 'Back to setup checklist' })); + await nextTick(); + expect(overlay).toBeInTheDocument(); + expect(overlay).toHaveTextContent('Connection form'); + expect(overlay).toHaveAttribute('inert'); + expect(overlay).toHaveAttribute('aria-hidden', 'true'); + expect(queryByRole('dialog')).toBeNull(); + unmount(); + }); + + it('shows a compact completion bar and lets users review completed items', async () => { + const { getByRole, queryByRole, emitted } = render(SetupPanel, { + props: { items: [{ id: 'slack', title: 'Slack', completed: true }], status: 'complete' }, + }); + expect(queryByRole('button', { name: 'Slack Complete' })).toBeNull(); + await userEvent.click(getByRole('button', { name: 'Setup complete' })); + expect(getByRole('button', { name: 'Slack Complete' })).toBeVisible(); + await userEvent.click(getByRole('button', { name: 'Execute' })); + expect(emitted('execute')).toHaveLength(1); + }); + + it.each(['validating', 'executing'] as const)('shows %s without an Execute action', (status) => { + const { getByRole, queryByRole } = render(SetupPanel, { + props: { items: [{ id: 'slack', title: 'Slack', completed: true }], status }, + }); + expect(getByRole('status')).toHaveTextContent( + status === 'validating' ? 'Validating…' : 'Executing…', + ); + expect(queryByRole('button', { name: 'Execute' })).toBeNull(); + }); + + it('opens the overlay by keyboard and returns focus to the selected row', async () => { + const activeItemId = ref(); + const { getByRole, getByText } = render({ + components: { SetupPanel }, + setup: () => ({ activeItemId, items: [{ id: 'slack', title: 'Slack', completed: false }] }), + template: ` + + `, + }); + getByRole('button', { name: 'Slack' }).focus(); + await userEvent.keyboard('{Enter}'); + expect(getByText('Connection form')).toBeVisible(); + const back = getByRole('button', { name: 'Back to setup checklist' }); + await waitFor(() => expect(back).toHaveFocus()); + await userEvent.keyboard('{Enter}'); + await waitFor(() => expect(getByRole('button', { name: 'Slack' })).toHaveFocus()); + }); + + it('keeps the checklist mounted but inaccessible while the overlay is open', async () => { + const activeItemId = ref(); + const { getByRole, getByTestId, queryByRole } = render({ + components: { SetupPanel }, + setup: () => ({ activeItemId, items: [{ id: 'slack', title: 'Slack', completed: false }] }), + template: ` + + `, + }); + await userEvent.click(getByRole('button', { name: 'Slack' })); + expect(getByRole('dialog', { name: 'Slack' })).toBeVisible(); + expect(getByTestId('setup-panel-row').closest('ul')).toHaveAttribute('inert'); + expect(queryByRole('button', { name: 'Slack' })).toBeNull(); + await userEvent.type(getByRole('textbox', { name: 'Channel' }), '#team'); + await userEvent.click(getByRole('textbox', { name: 'Chat' })); + expect(getByRole('textbox', { name: 'Channel' })).toHaveValue('#team'); + await userEvent.click(getByRole('textbox', { name: 'Channel' })); + await userEvent.keyboard('{Escape}'); + expect(queryByRole('dialog')).toBeNull(); + await waitFor(() => expect(getByRole('button', { name: 'Slack' })).toHaveFocus()); + }); + + it('renders a direct connection action without opening the overlay', async () => { + const connect = vi.fn(); + const { getByRole, queryByRole, emitted } = render(SetupPanel, { + props: { items: [{ id: 'slack', title: 'Slack', completed: false, hasAction: true }] }, + slots: { + action: { + template: '', + setup: () => ({ connect }), + }, + }, + }); + expect(getByRole('group', { name: 'Slack' })).toBeVisible(); + await userEvent.click(getByRole('button', { name: 'Connect' })); + expect(connect).toHaveBeenCalledOnce(); + expect(queryByRole('dialog')).toBeNull(); + expect(emitted('update:activeItemId')).toBeUndefined(); + }); + + it('exposes completion and prevents unavailable rows from opening', async () => { + const { getByRole, emitted } = render(SetupPanel, { + props: { + items: [ + { id: 'slack', title: 'Slack', completed: true }, + { id: 'details', title: 'Details', completed: false, disabled: true }, + ], + }, + }); + expect(getByRole('button', { name: 'Slack Complete' })).toBeEnabled(); + expect( + getByRole('button', { name: 'Slack Complete' }).querySelector( + '[data-icon="status-completed"]', + ), + ).toBeVisible(); + const details = getByRole('button', { name: 'Details' }); + expect(details).toBeDisabled(); + await userEvent.click(details); + expect(emitted('update:activeItemId')).toBeUndefined(); + }); + + it('returns to the remaining checklist when the active requirement disappears', async () => { + const { rerender, queryByText, getByRole, emitted } = render(SetupPanel, { + props: { items: [{ id: 'slack', title: 'Slack', completed: false }], activeItemId: 'slack' }, + slots: { detail: 'Connection form' }, + }); + await rerender({ items: [{ id: 'details', title: 'Details', completed: false }] }); + expect(queryByText('Connection form')).toBeNull(); + expect(getByRole('button', { name: 'Details' })).toBeVisible(); + await waitFor(() => expect(getByRole('button', { name: 'Details' })).toHaveFocus()); + expect(emitted('update:activeItemId')).toContainEqual([undefined]); + await rerender({ items: [] }); + expect(queryByText('Details')).toBeNull(); + }); + + it.each(['removed', 'disabled', 'empty'])( + 'emits one close event when the active item becomes %s', + async (change) => { + const items = [ + { id: 'slack', title: 'Slack', completed: false }, + { id: 'details', title: 'Details', completed: false }, + ]; + const { getByTestId, rerender, emitted } = render(SetupPanel, { + props: { items, activeItemId: 'slack' }, + global: { stubs: { transition: false } }, + }); + const overlay = getByTestId('setup-panel-overlay'); + overlay.style.transitionDuration = '0.05s'; + overlay.style.transitionDelay = '0s'; + await rerender({ + items: + change === 'empty' + ? [] + : change === 'removed' + ? items.slice(1) + : items.map((item) => ({ ...item, disabled: item.id === 'slack' })), + }); + const closeEventsBeforeLeave = emitted('detailClosed')?.length ?? 0; + await waitFor(() => expect(overlay).not.toBeInTheDocument()); + expect(emitted('detailClosed')).toEqual([[]]); + if (change !== 'empty') expect(closeEventsBeforeLeave).toBe(0); + }, + ); + + it.each(['missing', 'disabled', 'empty'])( + 'clears the %s initial selection without opening it after the items change', + async (initialState) => { + const activeItemId = ref('slack'); + const items = ref( + initialState === 'empty' + ? [] + : [ + { + id: initialState === 'missing' ? 'details' : 'slack', + title: 'Slack', + completed: false, + disabled: initialState === 'disabled', + }, + ], + ); + const onDetailClosed = vi.fn(); + const { queryByRole } = render({ + components: { SetupPanel }, + setup: () => ({ activeItemId, items, onDetailClosed }), + template: + '', + }); + expect(activeItemId.value).toBeUndefined(); + items.value = [{ id: 'slack', title: 'Slack', completed: false, disabled: false }]; + await nextTick(); + expect(queryByRole('dialog')).toBeNull(); + expect(onDetailClosed).not.toHaveBeenCalled(); + }, + ); + + it('does not open a selected detail when mounted during execution', () => { + const { queryByRole, getByRole, emitted } = render(SetupPanel, { + props: { + items: [{ id: 'slack', title: 'Slack', completed: true }], + activeItemId: 'slack', + status: 'executing', + }, + }); + expect(queryByRole('dialog')).toBeNull(); + expect(getByRole('status')).toHaveTextContent('Executing'); + expect(emitted('update:activeItemId')).toEqual([[undefined]]); + }); +}); diff --git a/packages/frontend/@n8n/design-system/src/components/N8nSetupPanel/SetupPanel.types.ts b/packages/frontend/@n8n/design-system/src/components/N8nSetupPanel/SetupPanel.types.ts new file mode 100644 index 00000000000..2c45cc510ac --- /dev/null +++ b/packages/frontend/@n8n/design-system/src/components/N8nSetupPanel/SetupPanel.types.ts @@ -0,0 +1,25 @@ +export interface SetupPanelItem { + /** Stable identity used to keep the detail open as items change. */ + id: string; + /** Service name or the label for a group of required fields. */ + title: string; + /** Short description of the values that remain to be configured. */ + subtitle?: string; + /** Whether all requirements represented by this row are configured. */ + completed: boolean; + /** Whether the host has enough context to open this item. */ + disabled?: boolean; + /** Whether the action slot replaces opening the detail card for this row. */ + hasAction?: boolean; +} + +export interface SetupPanelProps { + /** Checklist items in display order. */ + items: SetupPanelItem[]; + /** The item shown in the overlay, or undefined to show the checklist. */ + activeItemId?: string; + /** Current setup or test-execution state. Defaults to the checklist. */ + status?: 'incomplete' | 'validating' | 'complete' | 'executing'; + /** Whether another operation prevents starting a workflow test. */ + executeDisabled?: boolean; +} diff --git a/packages/frontend/@n8n/design-system/src/components/N8nSetupPanel/SetupPanel.vue b/packages/frontend/@n8n/design-system/src/components/N8nSetupPanel/SetupPanel.vue new file mode 100644 index 00000000000..055eba299f3 --- /dev/null +++ b/packages/frontend/@n8n/design-system/src/components/N8nSetupPanel/SetupPanel.vue @@ -0,0 +1,444 @@ + + + + + diff --git a/packages/frontend/@n8n/design-system/src/components/index.ts b/packages/frontend/@n8n/design-system/src/components/index.ts index 98e52809be6..ded2a567202 100644 --- a/packages/frontend/@n8n/design-system/src/components/index.ts +++ b/packages/frontend/@n8n/design-system/src/components/index.ts @@ -32,6 +32,10 @@ export { default as N8nAvatar } from './N8nAvatar'; export { default as N8nBadge } from './N8nBadge'; export { default as N8nBlockUi } from './N8nBlockUi'; export { default as N8nButton } from './N8nButton'; +export { default as N8nSetupPanel } from './N8nSetupPanel/SetupPanel.vue'; +export type { SetupPanelItem, SetupPanelProps } from './N8nSetupPanel/SetupPanel.types'; +export { default as N8nSetupConnection } from './N8nSetupConnection/SetupConnection.vue'; +export type { SetupConnectionProps } from './N8nSetupConnection/SetupConnection.types'; export { default as N8nButtonList } from './N8nButtonList'; export type { ButtonListOrientation, ButtonListProps } from './N8nButtonList'; export { default as N8nCallout } from './N8nCallout'; diff --git a/packages/frontend/@n8n/design-system/src/locale/lang/en.ts b/packages/frontend/@n8n/design-system/src/locale/lang/en.ts index 8dac5caf26d..810846159c1 100644 --- a/packages/frontend/@n8n/design-system/src/locale/lang/en.ts +++ b/packages/frontend/@n8n/design-system/src/locale/lang/en.ts @@ -2,6 +2,17 @@ import type { N8nLocale } from '../../types'; export default { + 'setupPanel.label': 'Workflow setup', + 'setupPanel.back': 'Back to setup checklist', + 'setupPanel.complete': 'Complete', + 'setupPanel.credentialSelected': 'Credential selected', + 'setupPanel.setupComplete': 'Setup complete', + 'setupPanel.execute': 'Execute', + 'setupPanel.validating': 'Validating…', + 'setupPanel.executing': 'Executing…', + 'setupPanel.connected': 'Connected', + 'setupPanel.changeConnection': 'Change connection', + 'setupPanel.moreOptions': 'More options', 'generic.retry': 'Retry', 'generic.cancel': 'Cancel', 'generic.close': 'Close', diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index 39ce62e2e61..049e1d0358f 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -7102,10 +7102,42 @@ "instanceAi.artifactsPanel.context.agentPreviewNamed": "{name} session", "instanceAi.artifactsPanel.context.agentPreviewTitled": "{agentName} — {sessionTitle}", "instanceAi.artifactsPanel.context.agentPreviewSessionTitled": "{sessionTitle}", - "instanceAi.setupPanel.applyError": "Could not apply the changes.", + "instanceAi.setupPanel.applyError": "Changes couldn't be saved. Try again.", "instanceAi.setupPanel.connected": "Connected", - "instanceAi.setupPanel.executeMessage": "Run a test execution of this workflow.", + "instanceAi.setupPanel.connection": "connection", + "instanceAi.setupPanel.details": "Details", + "instanceAi.setupPanel.needs": "Add {items}", + "instanceAi.setupPanel.connect": "Connect", + "instanceAi.setupPanel.advancedSetup": "Advanced setup", + "instanceAi.setupPanel.switchAccount": "Switch account", + "instanceAi.setupPanel.connectWithOAuth": "Connect with OAuth instead", + "instanceAi.setupPanel.replaceKey": "Create new credential", + "instanceAi.setupPanel.editCredential": "Edit credential", + "instanceAi.setupPanel.useExisting": "Use an existing credential", + "instanceAi.setupPanel.account": "Account", + "instanceAi.setupPanel.apiKey": "API key", + "instanceAi.setupPanel.credential": "Credential", + "instanceAi.setupPanel.accessDescription": "This workflow needs a credential to access {service}.", + "instanceAi.setupPanel.helpFindApiKey": "Help me find my API key", + "instanceAi.setupPanel.helpFindAccessToken": "Help me find my access token", + "instanceAi.setupPanel.helpSetUp": "Help me set this up", + "instanceAi.setupPanel.helpVisibleFields": "The OAuth form asks for: {fields}.", + "instanceAi.setupPanel.helpManagedOAuthAvailable": "Managed OAuth is available on this instance.", + "instanceAi.setupPanel.helpManagedOAuthUnavailable": "Managed OAuth isn't available on this instance. Help me configure my own OAuth app.", + "instanceAi.setupPanel.setCredentialsPerNode": "Set credentials per node", + "instanceAi.setupPanel.creditsDescription": "Use Gateway credits without adding an API key.", + "instanceAi.setupPanel.connectedWith": "Connected with", + "instanceAi.setupPanel.useOwnKey": "Use my API key", + "instanceAi.setupPanel.useCredits": "Use credits", + "instanceAi.setupPanel.manageCredits": "Manage Gateway credits", + "instanceAi.setupPanel.saveAndSignIn": "Save and sign in", + "instanceAi.setupPanel.redirectUrl": "Add this redirect URL to your app", + "instanceAi.setupPanel.copyRedirectUrl": "Copy redirect URL", + "instanceAi.setupPanel.connectionError": "Connection failed. Try again.", + "instanceAi.setupPanel.executedMessage": "The workflow execution finished with execution id \"{executionId}\"", + "instanceAi.setupPanel.executeError": "The workflow test couldn't start. Try again.", "instanceAi.setupPanel.ready": "Ready", + "node.setupRequired": "Add a credential in the setup panel", "instanceAi.projectSelect.tooltip": "Where n8n Assistant creates automations", "instanceAi.projectSelect.placeholder": "Select a project", "instanceAi.projectSelect.search": "Search projects", diff --git a/packages/frontend/editor-ui/src/app/composables/useInstanceAiEditorCapability.ts b/packages/frontend/editor-ui/src/app/composables/useInstanceAiEditorCapability.ts index 7b9cff801f8..efa9e8bc32b 100644 --- a/packages/frontend/editor-ui/src/app/composables/useInstanceAiEditorCapability.ts +++ b/packages/frontend/editor-ui/src/app/composables/useInstanceAiEditorCapability.ts @@ -31,6 +31,8 @@ export interface InstanceAiCredentialContext { documentationUrl?: string; /** OAuth redirect/callback URL shown in the modal, when this is an OAuth credential. */ oauthRedirectUrl?: string; + /** Instance-specific setup guidance appended to the help question. */ + setupContext?: string; } /** diff --git a/packages/frontend/editor-ui/src/app/composables/useRunWorkflow.ts b/packages/frontend/editor-ui/src/app/composables/useRunWorkflow.ts index 63296738970..5542f72d91f 100644 --- a/packages/frontend/editor-ui/src/app/composables/useRunWorkflow.ts +++ b/packages/frontend/editor-ui/src/app/composables/useRunWorkflow.ts @@ -57,7 +57,6 @@ import { useExecutionsStore } from '@/features/execution/executions/executions.s import { useTelemetry } from '@n8n/composables/useTelemetry'; import { useSettingsStore } from '@n8n/stores/settings.store'; import { useUIStore } from '@/app/stores/ui.store'; -import { usePushConnectionStore } from '@/app/stores/pushConnection.store'; import { useNodeDirtiness } from '@/app/composables/useNodeDirtiness'; import { useCanvasOperations } from './useCanvasOperations'; import { chatEventBus } from '@n8n/chat/event-buses'; @@ -65,6 +64,7 @@ import { useAgentRequestStore } from '@n8n/stores/useAgentRequestStore'; import { useWorkflowSaving } from './useWorkflowSaving'; import { useDocumentTitle } from './useDocumentTitle'; import { useEditorContext } from './useEditorContext'; +import { useRunWorkflowApi } from './useRunWorkflowApi'; import { useChat } from '@n8n/chat/composables'; import type { WorkflowObjectAccessors } from '../types'; @@ -89,8 +89,8 @@ export function useRunWorkflow(useRunWorkflowOpts: { const agentRequestStore = useAgentRequestStore(); const rootStore = useRootStore(); - const pushConnectionStore = usePushConnectionStore(); const workflowsStore = useWorkflowsStore(); + const workflowRunner = useRunWorkflowApi(); const workflowDocumentStore = useRunWorkflowOpts.workflowDocumentStore ?? injectWorkflowDocumentStore(); const workflowExecutionState = computed(() => @@ -127,35 +127,7 @@ export function useRunWorkflow(useRunWorkflowOpts: { // Starts to execute a workflow on server async function runWorkflowApi(runData: IStartRunData): Promise { - if (!pushConnectionStore.isConnected) { - // Do not start if the connection to server is not active - // because then it can not receive the data as it executes. - throw new Error(i18n.baseText('workflowRun.noActiveConnectionToTheServer')); - } - - // Set the execution as started, but still waiting for the execution to be retrieved - workflowExecutionState.value.setActiveExecutionId(null); - - let response: IExecutionPushResponse; - try { - response = await workflowsStore.runWorkflow(runData); - } catch (error) { - workflowExecutionState.value.setActiveExecutionId(undefined); - throw error; - } - - const workflowExecutionIdIsNew = - workflowExecutionState.value.previousExecutionId !== response.executionId; - const workflowExecutionIdIsPending = workflowExecutionState.value.activeExecutionId === null; - if (response.executionId && workflowExecutionIdIsNew && workflowExecutionIdIsPending) { - workflowExecutionState.value.setActiveExecutionId(response.executionId); - } - - if (response.waitingForWebhook === true) { - workflowExecutionState.value.setExecutionWaitingForWebhook(true); - } - - return response; + return await workflowRunner.runWorkflowApi(runData, workflowDocumentStore.value.documentId); } async function runWorkflow(options: { diff --git a/packages/frontend/editor-ui/src/app/composables/useRunWorkflowApi.ts b/packages/frontend/editor-ui/src/app/composables/useRunWorkflowApi.ts new file mode 100644 index 00000000000..4d9a485e345 --- /dev/null +++ b/packages/frontend/editor-ui/src/app/composables/useRunWorkflowApi.ts @@ -0,0 +1,46 @@ +import { useI18n } from '@n8n/i18n'; +import type { IStartRunData } from '@/Interface'; +import { usePushConnectionStore } from '@/app/stores/pushConnection.store'; +import type { WorkflowDocumentId } from '@/app/stores/workflowDocument.store'; +import { useWorkflowExecutionStateStore } from '@/app/stores/workflowExecutionState.store'; +import { useWorkflowsStore } from '@/app/stores/workflows.store'; +import type { IExecutionPushResponse } from '@/features/execution/executions/executions.types'; + +export function useRunWorkflowApi() { + const i18n = useI18n(); + const pushConnectionStore = usePushConnectionStore(); + const workflowsStore = useWorkflowsStore(); + + async function runWorkflowApi( + runData: IStartRunData, + documentId: WorkflowDocumentId, + ): Promise { + if (!pushConnectionStore.isConnected) { + throw new Error(i18n.baseText('workflowRun.noActiveConnectionToTheServer')); + } + + const executionState = useWorkflowExecutionStateStore(documentId); + // Push handlers only accept this run after the document starts tracking it. + executionState.setActiveExecutionId(null); + + let response: IExecutionPushResponse; + try { + response = await workflowsStore.runWorkflow(runData); + } catch (error) { + executionState.setActiveExecutionId(undefined); + throw error; + } + + if ( + response.executionId && + executionState.previousExecutionId !== response.executionId && + executionState.activeExecutionId === null + ) { + executionState.setActiveExecutionId(response.executionId); + } + if (response.waitingForWebhook === true) executionState.setExecutionWaitingForWebhook(true); + return response; + } + + return { runWorkflowApi }; +} diff --git a/packages/frontend/editor-ui/src/app/constants/durations.ts b/packages/frontend/editor-ui/src/app/constants/durations.ts index 2c021b94cc5..01b898fcad4 100644 --- a/packages/frontend/editor-ui/src/app/constants/durations.ts +++ b/packages/frontend/editor-ui/src/app/constants/durations.ts @@ -49,6 +49,9 @@ export const AGENT_EXTERNAL_UPDATE_NOTICE_DURATION = 5 * TIME.MINUTE; /** Keeps short agent tool and skill calls perceptible without adding continuous motion. */ export const AGENT_CAPABILITY_ACTIVE_MIN_DURATION = 300; +/** Keeps a completed setup item visible briefly before returning to the checklist. */ +export const SETUP_PANEL_SUCCESS_DELAY = 650; + /** Hover-intent delays for reveal-on-hover affordances (e.g. a collapsed group's description). */ export const HOVER_DELAY = { /** Delay before a hovered affordance reveals its content. */ diff --git a/packages/frontend/editor-ui/src/app/constants/injectionKeys.ts b/packages/frontend/editor-ui/src/app/constants/injectionKeys.ts index fc0ac3988ad..7908d352eaf 100644 --- a/packages/frontend/editor-ui/src/app/constants/injectionKeys.ts +++ b/packages/frontend/editor-ui/src/app/constants/injectionKeys.ts @@ -31,6 +31,8 @@ export const ExecutionDataStoreKey: InjectionKey< // derived from it via injectWorkflowExecutionStateStore(), so a subtree's // document scope and execution scope can never diverge. export const CanvasRenderDataKey: InjectionKey> = Symbol('CanvasRenderData'); +/** Keep setup hints compact while retaining their full text on the field. */ +export const CompactParameterHintsKey: InjectionKey = Symbol('CompactParameterHints'); /** * Opts resource-locator dropdowns into teleporting to ``. Defaults to * `false` (stay in the local stacking context, e.g. inside the NDV dialog). @@ -92,6 +94,8 @@ export type EditorEnabledFeatures = Partial> & { executionSuccessToasts?: boolean; executionErrorToasts?: boolean; executionButtonType?: 'primary' | 'secondary'; + /** Show missing credentials as setup warnings in hosts with a setup panel. */ + credentialSetupWarnings?: boolean; }; export const EditorEnabledFeaturesKey: InjectionKey>> = Symbol('EditorEnabledFeatures'); diff --git a/packages/frontend/editor-ui/src/app/constants/localStorage.ts b/packages/frontend/editor-ui/src/app/constants/localStorage.ts index a68772a7161..c91314b0c35 100644 --- a/packages/frontend/editor-ui/src/app/constants/localStorage.ts +++ b/packages/frontend/editor-ui/src/app/constants/localStorage.ts @@ -27,6 +27,16 @@ export const LOCAL_STORAGE_CHAT_HUB_HAD_CONVERSATION_BEFORE = (userId: string) = `${userId}_N8N_CHAT_HUB_HAD_CONVERSATION_BEFORE`; export const LOCAL_STORAGE_INSTANCE_AI_ARTIFACT_PREVIEW_OPEN = (threadId: string) => `N8N_INSTANCE_AI_ARTIFACT_PREVIEW_OPEN:${threadId}`; +export const LOCAL_STORAGE_INSTANCE_AI_SETUP_ITEMS = ( + userId: string, + threadId: string, + workflowId: string, +) => `N8N_INSTANCE_AI_SETUP_ITEMS:${userId}:${threadId}:${workflowId}`; +export const LOCAL_STORAGE_INSTANCE_AI_SETUP_DISMISSED = ( + userId: string, + threadId: string, + workflowId: string, +) => `N8N_INSTANCE_AI_SETUP_DISMISSED:${userId}:${threadId}:${workflowId}`; export const LOCAL_STORAGE_INSTANCE_AI_CHAT_PANEL_WIDTH_RATIO = 'N8N_INSTANCE_AI_CHAT_PANEL_WIDTH_RATIO'; export const LOCAL_STORAGE_SIDEBAR_WIDTH = 'N8N_SIDEBAR_WIDTH'; diff --git a/packages/frontend/editor-ui/src/app/constants/nodeTypes.ts b/packages/frontend/editor-ui/src/app/constants/nodeTypes.ts index 4ab1d7ca8df..e4836f2e2b0 100644 --- a/packages/frontend/editor-ui/src/app/constants/nodeTypes.ts +++ b/packages/frontend/editor-ui/src/app/constants/nodeTypes.ts @@ -91,6 +91,15 @@ export const MARKDOWN_NODE_TYPE = 'n8n-nodes-base.markdown'; export const XML_NODE_TYPE = 'n8n-nodes-base.xml'; export const CRYPTO_NODE_TYPE = 'n8n-nodes-base.crypto'; export const RSS_READ_NODE_TYPE = 'n8n-nodes-base.rssFeedRead'; + +// These nodes can use arbitrary providers. Gateway credentials require a known provider. +export const AI_GATEWAY_UNSUPPORTED_NODE_TYPES: readonly string[] = [ + 'n8n-nodes-base.httpRequest', + 'n8n-nodes-base.httpRequestTool', + '@n8n/n8n-nodes-langchain.toolHttpRequest', + 'n8n-nodes-base.graphql', + 'n8n-nodes-base.graphqlTool', +]; export const COMPRESSION_NODE_TYPE = 'n8n-nodes-base.compression'; export const EDIT_IMAGE_NODE_TYPE = 'n8n-nodes-base.editImage'; export const CHAIN_SUMMARIZATION_LANGCHAIN_NODE_TYPE = diff --git a/packages/frontend/editor-ui/src/features/ai/chatHub/composables/useChatHubMarkdownOptions.test.ts b/packages/frontend/editor-ui/src/features/ai/chatHub/composables/useChatHubMarkdownOptions.test.ts index 93ca446f4ca..e87c01e0ceb 100644 --- a/packages/frontend/editor-ui/src/features/ai/chatHub/composables/useChatHubMarkdownOptions.test.ts +++ b/packages/frontend/editor-ui/src/features/ai/chatHub/composables/useChatHubMarkdownOptions.test.ts @@ -8,7 +8,7 @@ import { function renderMarkdown(content: string): string { const markdown = useChatHubMarkdownOptions('code-actions', 'table-container', null); - const renderer = new MarkdownIt(); + const renderer = new MarkdownIt(markdown.options); for (const plugin of markdown.plugins.value) { renderer.use(plugin); @@ -18,6 +18,12 @@ function renderMarkdown(content: string): string { } describe('useChatHubMarkdownOptions', () => { + it('makes plain URLs clickable while preserving code examples', () => { + const html = renderMarkdown('Open https://api.slack.com/apps or copy `https://example.test`.'); + expect(html).toContain('href="https://api.slack.com/apps" target="_blank" rel="noopener"'); + expect(html).toContain('https://example.test'); + }); + it('renders app-relative links without a new-tab target', () => { const html = renderMarkdown('[Preview](/projects/project-1/agents/agent-1/preview)'); diff --git a/packages/frontend/editor-ui/src/features/ai/chatHub/composables/useChatHubMarkdownOptions.ts b/packages/frontend/editor-ui/src/features/ai/chatHub/composables/useChatHubMarkdownOptions.ts index cd1a7e50892..2e612bfdd7d 100644 --- a/packages/frontend/editor-ui/src/features/ai/chatHub/composables/useChatHubMarkdownOptions.ts +++ b/packages/frontend/editor-ui/src/features/ai/chatHub/composables/useChatHubMarkdownOptions.ts @@ -82,6 +82,7 @@ export function useChatHubMarkdownOptions( const options = { breaks: true, + linkify: true, highlight(str: string, lang: string) { if (!lang) { return ''; // use external default escaping diff --git a/packages/frontend/editor-ui/src/features/ai/instanceAi/InstanceAiThreadView.vue b/packages/frontend/editor-ui/src/features/ai/instanceAi/InstanceAiThreadView.vue index 2fa87f8e838..8a1a7fcfac3 100644 --- a/packages/frontend/editor-ui/src/features/ai/instanceAi/InstanceAiThreadView.vue +++ b/packages/frontend/editor-ui/src/features/ai/instanceAi/InstanceAiThreadView.vue @@ -220,8 +220,7 @@ watch( { immediate: true }, ); // --- Setup panel (checklist docked above the composer) --- -// Anchors to the active canvas tab's workflow; on a hydrated thread with no -// tab state yet, the latest workflow artifact wins (insertion order). +// Early setup announcements can arrive before the first workflow artifact. const setupPanelWorkflowId = computed(() => { if (!settingsStore.isInstanceAiSetupPanelEnabled) return undefined; const active = preview.activeWorkflowId.value; @@ -230,13 +229,14 @@ const setupPanelWorkflowId = computed(() => { for (const entry of thread.producedArtifacts.values()) { if (entry.type === 'workflow') latest = entry.id; } - return latest; + return latest ?? thread.latestSetupWorkflowId; }); const setupPanelProjectId = computed(() => setupPanelWorkflowId.value ? thread.producedArtifacts.get(setupPanelWorkflowId.value)?.projectId : undefined, ); +const setupOverlapHeight = ref(0); const agentReturnContext = useAgentReturnContextStore().consumePendingArtifactReturn(); const agentReturnWorkflowId = agentReturnContext?.workflowId; @@ -809,6 +809,7 @@ async function persistTestAgentOfferDismissal(agentId: string) { > @@ -817,6 +818,7 @@ async function persistTestAgentOfferDismissal(agentId: string) { v-if="setupPanelWorkflowId" :workflow-id="setupPanelWorkflowId" :project-id="setupPanelProjectId" + @update:overlap-height="setupOverlapHeight = $event" />