mirror of
https://github.com/n8n-io/n8n.git
synced 2026-10-17 05:29:44 +02:00
chore: Improve Cubic performance and configuration clarity (#37193)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Matsu
GitHub
parent
c678970930
commit
4f36a2d96b
80
.agents/review-rules/README.md
Normal file
80
.agents/review-rules/README.md
Normal file
|
|
@ -0,0 +1,80 @@
|
|||
# Review rules
|
||||
|
||||
Rules the AI reviewer (cubic) enforces on pull requests. One rule per file,
|
||||
grouped by the agent that loads it. `cubic.yaml` links these files via
|
||||
`file_paths`; the prose in `cubic.yaml` stays thin so the rules are reviewable
|
||||
as normal markdown.
|
||||
|
||||
## Layout
|
||||
|
||||
| Directory | Agent | Scope |
|
||||
|-------------|----------|-----------------------------------------------------|
|
||||
| `security/` | Security | backend packages + nodes |
|
||||
| `backend/` | Backend | `cli`, `@n8n/db`, `core`, `workflow`, node packages |
|
||||
| `frontend/` | Frontend | `packages/frontend` |
|
||||
| `qa-dx/` | QA & DX | `.github`, `docker`, `scripts`, `patches`, `packages/testing`, the lint/test/TS config packages, baselines |
|
||||
|
||||
One slot of five is left. QA & DX covers the build, test, and CI surface — the
|
||||
same paths `.github/OWNERS` assigns to `@n8n-io/qa-dx`. Code-quality rules that
|
||||
happen to apply broadly (error classes, `any`, lazy imports) are backend rules,
|
||||
not QA & DX ones.
|
||||
|
||||
## Limits that bite
|
||||
|
||||
cubic fails silently on all three of these, which is why `pnpm check:cubic-config`
|
||||
enforces them in CI:
|
||||
|
||||
- **5 enabled agents per repository.** Rules past the fifth never run and cubic
|
||||
says nothing. One slot is deliberately left free.
|
||||
- **10,000 characters per agent**, counting the `description` plus every linked
|
||||
file, concatenated in the listed order. Everything past the limit is dropped
|
||||
from the review prompt.
|
||||
- **Repo-relative file paths only.** Globs, directories, parent-directory
|
||||
traversal, and absolute paths are all rejected — list each file explicitly.
|
||||
The schema caps `file_paths` at 10 entries per agent.
|
||||
|
||||
## Adding a rule
|
||||
|
||||
1. Write the file in the directory for the agent that should own it. Open with a
|
||||
one-line "Applies to:" so the reviewer skips it on unrelated files — a
|
||||
backend PR still loads the node rules, since include globs are per-agent.
|
||||
2. Add its path to that agent's `file_paths` in `cubic.yaml`.
|
||||
3. Run `pnpm check:cubic-config`. It validates `cubic.yaml` against cubic's
|
||||
published JSON schema, then fails on a missing path, an over-budget agent, or
|
||||
a rule file nobody links, and warns at 80% of the ceiling.
|
||||
|
||||
The schema is vendored at `.github/scripts/quality/cubic-config.schema.json` so
|
||||
the check needs no network. `util-refresh-cubic-schema.yml` re-pulls it on the 1st
|
||||
of each month and opens a PR when it changed — review that diff for new cubic
|
||||
options worth adopting. To refresh by hand:
|
||||
`node .github/scripts/quality/check-cubic-config.mjs --refresh`.
|
||||
|
||||
Don't restate something ESLint already errors on — see the "Don't repeat the
|
||||
linter" section in `cubic.yaml`. Write what static analysis cannot see.
|
||||
|
||||
## Working with cubic on a PR
|
||||
|
||||
Tag `@cubic-dev-ai` in a PR comment (GitHub autocomplete won't offer it — type it).
|
||||
Replying to one of cubic's own comments needs no tag, but a bare reply never
|
||||
authorises code changes.
|
||||
|
||||
| Ask | Comment |
|
||||
|-----|---------|
|
||||
| Re-review everything | `@cubic-dev-ai review this PR` |
|
||||
| Review only what changed since its last pass | `@cubic-dev-ai incremental review` |
|
||||
| Deeper pass with the stronger models | `@cubic-dev-ai ultrareview` — or `ultrareview: focus on <topic>` |
|
||||
| Fix a finding on this branch | `@cubic-dev-ai fix this issue in this branch` |
|
||||
| One-off context for this run only | `@cubic-dev-ai review this and use <url>` |
|
||||
| Ask about a finding | reply in the thread |
|
||||
|
||||
Two things worth knowing:
|
||||
|
||||
- Ultrareview is billed at 3× a normal review, which is why it is manual rather
|
||||
than automatic here.
|
||||
- A PR over 10,000 changed lines, or carrying one of the ignored automation
|
||||
labels, gets no automatic review. `@cubic-dev-ai review this PR` overrides
|
||||
that when you want it.
|
||||
|
||||
Disagreeing in a thread is not wasted: cubic turns feedback into team-scoped
|
||||
learnings, so a well-argued "this is fine because X" shapes later reviews. Vague
|
||||
replies ("done", "thanks") don't.
|
||||
|
|
@ -0,0 +1,59 @@
|
|||
# Controller request bodies must use a DTO
|
||||
|
||||
Applies to: `*.controller.ts` outside `packages/cli/src/public-api` (the public
|
||||
API has its own lint rules).
|
||||
|
||||
Controller endpoints that accept a request body must use the `@Body` decorator
|
||||
with a DTO class. TypeScript types alone provide no runtime validation — only
|
||||
`@Body` with a Zod-based DTO (extending `Z.class`) validates incoming data.
|
||||
|
||||
This fails quietly rather than loudly: `controller.registry.ts` validates only
|
||||
when the parameter type exposes `safeParse`, and otherwise pushes nothing into
|
||||
the handler's arguments at all.
|
||||
|
||||
Only flag when there is positive evidence the developer intended to accept a
|
||||
body but didn't use the pattern:
|
||||
|
||||
1. A parameter named `payload`, `body`, or `data` without `@Body`
|
||||
2. Direct `req.body` access in a controller method
|
||||
3. `@Body` with a type not ending in `Dto` — suggests unawareness of the pattern
|
||||
|
||||
Do NOT flag:
|
||||
|
||||
- `@Body` with a type ending in `Dto` — the developer knows the pattern
|
||||
- POST/PUT/PATCH endpoints with no body parameter; they may legitimately have no body
|
||||
- Webhook controllers
|
||||
|
||||
Violation:
|
||||
|
||||
```typescript
|
||||
@Post('/')
|
||||
async create(req: AuthenticatedRequest, payload: CreateUser) {
|
||||
// payload is undefined - missing @Body decorator
|
||||
}
|
||||
|
||||
@Post('/')
|
||||
async create(req: AuthenticatedRequest) {
|
||||
const data = req.body; // No runtime validation
|
||||
}
|
||||
|
||||
@Post('/')
|
||||
async create(@Body payload: CreateUser) {
|
||||
// Type doesn't end in Dto - likely missing Zod validation
|
||||
}
|
||||
```
|
||||
|
||||
Allowed:
|
||||
|
||||
```typescript
|
||||
@Post('/')
|
||||
async create(@Body payload: CreateUserDto) { ... }
|
||||
|
||||
@Post('/activate')
|
||||
async activate(req: AuthenticatedRequest) {
|
||||
// Legitimately no body needed
|
||||
}
|
||||
```
|
||||
|
||||
Use a DTO from `@n8n/api-types` or a local `dto/` directory. DTOs must extend
|
||||
`Z.class` from `zod-class` for runtime validation.
|
||||
15
.agents/review-rules/backend/error-classes.md
Normal file
15
.agents/review-rules/backend/error-classes.md
Normal file
|
|
@ -0,0 +1,15 @@
|
|||
# Throw a typed error, not a plain Error
|
||||
|
||||
Applies to: backend packages (`cli`, `@n8n/db`, `core`, `workflow`) and the node packages.
|
||||
|
||||
The `no-plain-errors` lint rule is switched off repo-wide, so nothing catches
|
||||
this. Flag `throw new Error(...)` in new code and pick by cause:
|
||||
|
||||
- `UserError` — the user caused it: invalid input, unauthorized action,
|
||||
business-rule violation
|
||||
- `OperationalError` — transient and expected: a failing network request, a DB
|
||||
timeout; something to handle gracefully
|
||||
- `UnexpectedError` — a bug: logic mistake, unhandled case, failed assertion
|
||||
|
||||
Do not flag `ApplicationError`; the `no-application-error` ESLint rule already
|
||||
fails the build for it.
|
||||
10
.agents/review-rules/backend/explicit-any.md
Normal file
10
.agents/review-rules/backend/explicit-any.md
Normal file
|
|
@ -0,0 +1,10 @@
|
|||
# `any` where a real type is available
|
||||
|
||||
Applies to: `packages/cli`, `packages/workflow`, `packages/nodes-base`,
|
||||
`packages/@n8n/nodes-langchain`.
|
||||
|
||||
`@typescript-eslint/no-explicit-any` is downgraded to a warning in these
|
||||
packages, and `pnpm lint` runs `--quiet`, so warnings never surface.
|
||||
|
||||
Flag a new `any` there when `unknown` plus a type guard, or an existing
|
||||
interface, would do. Do not flag `any` in test code.
|
||||
22
.agents/review-rules/backend/hand-rolled-delays.md
Normal file
22
.agents/review-rules/backend/hand-rolled-delays.md
Normal file
|
|
@ -0,0 +1,22 @@
|
|||
# Hand-rolled delays the linter cannot see
|
||||
|
||||
Applies to: backend packages (`cli`, `@n8n/db`, `core`, `workflow`) and the node packages.
|
||||
|
||||
`no-restricted-sleep-definition` catches only helpers literally named `sleep` or
|
||||
`sleepWithAbort`, so it misses the same thing under another name.
|
||||
|
||||
Flag a delay re-implemented as `wait`, `delay`, `pause`, `waitFor`, … whose
|
||||
entire body is a promise wrapping `setTimeout`, and an inline awaited
|
||||
`new Promise((resolve) => setTimeout(resolve, ms))`.
|
||||
|
||||
Use `sleep(ms, abortSignal?)` from `@n8n/utils/sleep`. It takes the abort signal,
|
||||
so an abort-aware local waiter is a violation too.
|
||||
|
||||
Do NOT flag:
|
||||
|
||||
- Wrappers that do more than wait: racing another promise, rejecting with a
|
||||
timeout error, retry backoff, debounce/throttle
|
||||
- Bare `setTimeout` used for scheduling rather than awaited as a delay
|
||||
- Fake timer helpers such as `vi.advanceTimersByTime`
|
||||
- `packages/@n8n/node-cli/**` and `packages/@n8n/typeorm/**`, which cannot depend
|
||||
on `@n8n/utils`
|
||||
11
.agents/review-rules/backend/lazy-load-heavy-modules.md
Normal file
11
.agents/review-rules/backend/lazy-load-heavy-modules.md
Normal file
|
|
@ -0,0 +1,11 @@
|
|||
# Lazy-load heavy or native modules
|
||||
|
||||
Applies to: backend packages (`cli`, `@n8n/db`, `core`, `workflow`) and the node packages.
|
||||
|
||||
A top-level `import` of a module used only on a specific code path loads it into
|
||||
every process at startup, raising baseline memory. Native modules (e.g.
|
||||
`isolated-vm`) can crash instances that lack the binary. Large parsers (e.g.
|
||||
`jsdom`, ~16 MB heap) waste memory when the path is rarely hit.
|
||||
|
||||
Fix: `await import()` at point of use. For barrel files, use `export type`
|
||||
instead of a value re-export when consumers only need the type.
|
||||
15
.agents/review-rules/frontend/design-system.md
Normal file
15
.agents/review-rules/frontend/design-system.md
Normal file
|
|
@ -0,0 +1,15 @@
|
|||
# Design system enforcement
|
||||
|
||||
Applies to: `packages/frontend`.
|
||||
|
||||
The design system skill linked alongside this file is the source of truth for
|
||||
which token to reach for. This file sets the enforcement level.
|
||||
|
||||
Stylelint validates CSS custom-property *names* but never their values, so
|
||||
nothing catches a hard-coded one.
|
||||
|
||||
- Strong warning: hard-coded visual values (px, rem, hex colours, durations)
|
||||
where a token exists; legacy token usage; deprecated style or component
|
||||
surfaces.
|
||||
- Soft warning: token-to-token substitutions. Ask for intent rather than
|
||||
asserting a regression — a deliberate change looks identical to a mistake.
|
||||
37
.agents/review-rules/frontend/workflow-document-store.md
Normal file
37
.agents/review-rules/frontend/workflow-document-store.md
Normal file
|
|
@ -0,0 +1,37 @@
|
|||
# Migrated workflow fields must use workflowDocumentStore
|
||||
|
||||
Applies to: `packages/frontend`.
|
||||
|
||||
Workflow state is actively migrating from `workflowsStore`
|
||||
(`packages/frontend/editor-ui/src/app/stores/workflows.store.ts`) to
|
||||
`workflowDocumentStore`
|
||||
(`packages/frontend/editor-ui/src/app/stores/workflowDocument.store.ts`).
|
||||
|
||||
`workflowDocumentStore` is the single source of truth for migrated fields. New
|
||||
code that reads or writes one of them must go through it, not through
|
||||
`workflowsStore.workflow.*` or an equivalent accessor.
|
||||
|
||||
Already-migrated fields — do NOT access these via `workflowsStore`:
|
||||
|
||||
`active`, `activeVersion`, `activeVersionId`, `checksum`, `createdAt`,
|
||||
`homeProject`, `meta`, `pinData`, `settings`, `tags`, `updatedAt`
|
||||
|
||||
Flag when NEW code does any of:
|
||||
|
||||
1. Reads a migrated field from `workflowsStore.workflow.<field>`
|
||||
2. Reads one from a destructured `workflow` ref originating in `workflowsStore`
|
||||
3. Writes one through `workflowsStore`, e.g. `workflowsStore.workflow.active = true`
|
||||
4. Calls a `workflowsStore` setter/action that only mutates a migrated field,
|
||||
when `workflowDocumentStore` already exposes an equivalent method
|
||||
|
||||
Do NOT flag:
|
||||
|
||||
- Non-migrated fields, e.g. `workflowsStore.workflow.nodes`, `.connections`
|
||||
- Code inside `workflowDocument.store.ts` or its sub-modules — those *are* the
|
||||
source of truth
|
||||
- Code inside `workflows.store.ts` itself — the migration is still in progress there
|
||||
|
||||
Use `useWorkflowDocumentStore(createWorkflowDocumentId(workflowId))` from
|
||||
`@/app/stores/workflowDocument.store.ts`.
|
||||
|
||||
This is an active migration; the field list will grow.
|
||||
32
.agents/review-rules/qa-dx/docker-image-pinning.md
Normal file
32
.agents/review-rules/qa-dx/docker-image-pinning.md
Normal file
|
|
@ -0,0 +1,32 @@
|
|||
# Base images and pinning
|
||||
|
||||
Applies to: `docker/images/**/Dockerfile*`.
|
||||
|
||||
## A tag and its digest change together
|
||||
|
||||
Base images are pinned as `tag@sha256:...`. The tag is documentation; the digest
|
||||
is what actually resolves. Flag a diff that edits one without the other — a
|
||||
bumped tag with a stale digest silently keeps building the old image, and a
|
||||
bumped digest under an old tag makes the Dockerfile lie about what it runs.
|
||||
|
||||
```dockerfile
|
||||
ARG BUILDER_IMAGE=node:26.7.0-alpine3.24@sha256:aadf41...
|
||||
ARG RUNTIME_IMAGE=n8nio/base:26.7.0@sha256:336873...
|
||||
```
|
||||
|
||||
Also flag a base image reference that loses its digest entirely, and a
|
||||
`FROM`/`ARG` that introduces a floating tag such as `latest`, `alpine`, or a
|
||||
major-only version.
|
||||
|
||||
## Node version consistency
|
||||
|
||||
`ARG NODE_VERSION`, the builder tag, and the runtime base tag describe the same
|
||||
Node release. Flag a bump that moves one and leaves another behind.
|
||||
|
||||
## The runtime base only changes here
|
||||
|
||||
A separate workflow builds `n8nio/base`; a new base reaches the n8n image only
|
||||
when `RUNTIME_IMAGE` changes in this file. Flag a PR that expects a base change
|
||||
to arrive on its own, and flag a runtime stage that installs a compiler or
|
||||
build dependency — the runtime base ships without one deliberately, and build
|
||||
tooling belongs in a builder stage that is discarded.
|
||||
46
.agents/review-rules/qa-dx/docker-native-modules.md
Normal file
46
.agents/review-rules/qa-dx/docker-native-modules.md
Normal file
|
|
@ -0,0 +1,46 @@
|
|||
# Native module builds in the Docker images
|
||||
|
||||
Applies to: `docker/images/**/Dockerfile*`.
|
||||
|
||||
n8n compiles `sqlite3`, `isolated-vm`, and `@confluentinc/kafka-javascript`
|
||||
inside the image. Each line of that setup exists because a specific build broke.
|
||||
Flag a change that undoes one.
|
||||
|
||||
## Never `npm rebuild` a native module in a pnpm tree
|
||||
|
||||
A pnpm `node_modules` reaches the same package through several symlinks, and
|
||||
`npm rebuild` runs one install script per link, concurrently, in the same store
|
||||
directory. They collide on `build/node_gyp_bins` and on each other's make
|
||||
output. This produced an intermittent Docker build failure twice, for sqlite3
|
||||
and for isolated-vm.
|
||||
|
||||
Call node-gyp once, directly:
|
||||
|
||||
```dockerfile
|
||||
node /usr/local/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js rebuild --release
|
||||
```
|
||||
|
||||
Flag `npm rebuild <pkg>` or `npm install` in a Dockerfile stage that builds a
|
||||
native module.
|
||||
|
||||
## Use npm's bundled node-gyp, not `npx node-gyp`
|
||||
|
||||
`npx node-gyp` resolves a version at build time, so the toolchain drifts from
|
||||
the pinned image digest and builds stop being reproducible. The path above
|
||||
pins node-gyp to whatever the base image ships. Flag `npx node-gyp`.
|
||||
|
||||
## Do not reintroduce prebuilds
|
||||
|
||||
`node-gyp-build` prefers `prebuilds/` over `build/Release`, and reads
|
||||
`/etc/alpine-release` to detect musl. The hardened Alpine base has no such
|
||||
file, so the loader picks the glibc prebuild and the module crashes at runtime.
|
||||
The `rm -rf prebuilds` steps are load-bearing — flag their removal, and flag a
|
||||
new native module added without one.
|
||||
|
||||
## Keep the native compile above `COPY ./compiled`
|
||||
|
||||
`isolated-vm` builds in its own stage whose only cache input is the module
|
||||
source, specifically so an application change does not trigger a recompile.
|
||||
Flag a `COPY` of application code moved above a native build step, or a native
|
||||
build folded into the stage that copies `./compiled` — it silently turns a
|
||||
cached layer into a multi-minute compile on every build.
|
||||
35
.agents/review-rules/qa-dx/ratchets-and-allowlists.md
Normal file
35
.agents/review-rules/qa-dx/ratchets-and-allowlists.md
Normal file
|
|
@ -0,0 +1,35 @@
|
|||
# Ratchets and allowlists
|
||||
|
||||
Applies to: baseline files, `packages/cli/eslint.config.mjs`, lint configs.
|
||||
|
||||
Several checks are ratchets: they permit what already exists and fail only on
|
||||
growth. That design has one blind spot — adding the new violation to the
|
||||
baseline is indistinguishable, to the tool, from fixing it. Only review catches
|
||||
it.
|
||||
|
||||
Flag a diff that **adds** entries to any of these, and ask for the fix instead:
|
||||
|
||||
| File | Ratchet |
|
||||
|------|---------|
|
||||
| `.code-health-baseline.json` | `@n8n/code-health` violations |
|
||||
| `.boundaries-baseline.json` | `turbo boundaries` issue count |
|
||||
| `packages/testing/playwright/.janitor-baseline.json` | Playwright janitor findings |
|
||||
| `packages/cli/eslint.config.mjs` | the `misplaced-n8n-typeorm-import` and public-API allowlists, each captioned "NEVER add to this list" |
|
||||
|
||||
Removals are the healthy direction and need no comment.
|
||||
|
||||
Two specifics:
|
||||
|
||||
- `.boundaries-baseline.json` holds a single integer, not a fingerprinted list.
|
||||
A fixed issue and a new one at the same count cancel out and the ratchet stays
|
||||
green. Treat any change to that number as worth explaining.
|
||||
- Regenerating a baseline wholesale, rather than appending to it, hides growth
|
||||
inside a large diff. If a PR rewrites a baseline, ask what the net change to
|
||||
the violation count is.
|
||||
|
||||
## Downgrading a rule is the same move
|
||||
|
||||
Flag a lint rule moved from `'error'` to `'warn'` or `'off'`, and a new
|
||||
`eslint-disable` for one of the guarded rules. Most packages run
|
||||
`eslint . --quiet`, so a warning never fails CI — downgrading to `'warn'` is
|
||||
functionally deleting the rule while appearing to keep it.
|
||||
46
.agents/review-rules/qa-dx/workflow-safety.md
Normal file
46
.agents/review-rules/qa-dx/workflow-safety.md
Normal file
|
|
@ -0,0 +1,46 @@
|
|||
# GitHub Actions safety
|
||||
|
||||
Applies to: `.github/workflows/**`, `.github/actions/**`, `.poutine.yml`.
|
||||
|
||||
Poutine and Zizmor already fail CI for the classic hazards — `pull_request_target`
|
||||
(also an error-level `@n8n/code-health` rule), actions unpinned from a full commit
|
||||
SHA, untrusted checkout execution, self-hosted runner exposure. Do not repeat
|
||||
them. What follows is what the scanners cannot see.
|
||||
|
||||
## Silencing a scanner is the change to review
|
||||
|
||||
`.poutine.yml` carries two allowlists — `github_action_from_unverified_creator_used`
|
||||
(by purl) and `untrusted_checkout_exec` (by path). The file says "Add new
|
||||
entries only after security review". Nothing enforces that.
|
||||
|
||||
Flag any addition to `.poutine.yml`'s `skip:` block, any new `zizmor: ignore`
|
||||
comment, and any new entry in `.github/poutine-rules/`'s exceptions. Ask what
|
||||
was reviewed and why the finding is a false positive rather than a real one.
|
||||
The entry needs a comment giving the reason, matching the ones already there.
|
||||
|
||||
## A gate that cannot fail is not a gate
|
||||
|
||||
Required checks are assembled by the `ci-filter` action in `mode: validate`,
|
||||
fed from a `needs:` list. Flag:
|
||||
|
||||
- `continue-on-error: true` on a job that appears in a required-checks `needs:`
|
||||
list, or on the final validate step. It makes the gate green regardless of
|
||||
outcome. `continue-on-error` on a nightly or a notification step is fine.
|
||||
- A job removed from a required-checks `needs:` list without the PR saying why.
|
||||
- `if:` conditions on a required job that can silently skip it. A skipped
|
||||
required job and a passing one are hard to tell apart in the merge queue.
|
||||
|
||||
## Path filters decide whether a test runs at all
|
||||
|
||||
Jobs gated on a `ci-filter` filter only run when a matching file changed, so a
|
||||
filter that is too narrow means the test quietly stops covering new code. When
|
||||
a PR adds a directory that an existing filter was meant to cover — a new package
|
||||
under `packages/testing/`, a new script under `.github/scripts/` — check the
|
||||
filter still matches it.
|
||||
|
||||
## Least privilege
|
||||
|
||||
Every workflow declares a top-level `permissions:` block; without one it runs
|
||||
with the repository's broad default token. Flag a new workflow that omits it,
|
||||
and a job that widens `permissions` beyond what its steps use. A job calling a
|
||||
reusable workflow must grant at least what that workflow declares.
|
||||
36
.agents/review-rules/security/auth-and-access-control.md
Normal file
36
.agents/review-rules/security/auth-and-access-control.md
Normal file
|
|
@ -0,0 +1,36 @@
|
|||
# Authentication, authorization, and licensing
|
||||
|
||||
Applies to: `packages/cli`, `packages/@n8n/db`.
|
||||
|
||||
## Authentication and sessions
|
||||
|
||||
- JWT handling that weakens validation or expiration
|
||||
- Missing or disabled cookie security flags (HttpOnly, Secure, SameSite)
|
||||
- Changes that bypass MFA enforcement
|
||||
- SSO/SAML/OIDC flows with validation gaps
|
||||
- OAuth 2.0 missing PKCE, or with weak redirect URI validation
|
||||
|
||||
## Authorization
|
||||
|
||||
No static check covers this: the `endpoint-scope-coverage` rule in
|
||||
`@n8n/code-health` is disabled, so a new route without a scope decorator reaches
|
||||
production unnoticed unless a reviewer catches it.
|
||||
|
||||
- Every authenticated route needs `@GlobalScope` or `@ProjectScope`, unless it
|
||||
declares `skipAuth`, `allowUnauthenticated`, or `apiKeyAuth`
|
||||
- Missing credential permission validation before workflow execution
|
||||
- Subworkflow execution bypassing caller policies or ownership validation
|
||||
- Missing project-level or resource-level access controls
|
||||
|
||||
## License enforcement
|
||||
|
||||
- Missing `FeatureNotLicensedError` for unlicensed feature access
|
||||
- Bypassed license quota checks
|
||||
- New licensed features without middleware or module-level enforcement
|
||||
- New endpoints in `*.ee.ts` that reach licensed features without `@Licensed`
|
||||
- `@Licensed` not matching the feature (check `LICENSE_FEATURES` in `@n8n/constants`)
|
||||
- Endpoints in `*.ee.ts` carrying a scope decorator but missing `@Licensed` — the
|
||||
license check is separate from the permission check
|
||||
- Prefer the `@Licensed` decorator over custom licensing middleware
|
||||
- Decorator order: route decorator → `@Licensed` → scope decorators
|
||||
- Enterprise code reachable outside `*.ee.ts` files or licensed modules
|
||||
|
|
@ -0,0 +1,15 @@
|
|||
# Code execution and sandboxing
|
||||
|
||||
Applies to: `packages/cli`, `packages/core`, `packages/workflow`.
|
||||
|
||||
Flag changes that:
|
||||
|
||||
- Weaken expression evaluation sandbox protections
|
||||
- Reduce isolation in Code node sandboxes (JavaScript/Python)
|
||||
- Grant sandboxed contexts new access to Node.js builtins or external modules
|
||||
- Introduce prototype pollution or constructor access bypasses
|
||||
- Weaken prototype sanitizers or function context validators
|
||||
- Expose `process.env` access in Code nodes
|
||||
|
||||
Higher scrutiny applies to the expression engine and the code execution nodes —
|
||||
a regression there is reachable by any workflow author.
|
||||
18
.agents/review-rules/security/credentials-and-secrets.md
Normal file
18
.agents/review-rules/security/credentials-and-secrets.md
Normal file
|
|
@ -0,0 +1,18 @@
|
|||
# Credentials and secrets
|
||||
|
||||
Applies to: all backend packages.
|
||||
|
||||
Flag:
|
||||
|
||||
- Credentials logged or exposed in error messages
|
||||
- Hardcoded secrets, API keys, or tokens
|
||||
- Changes to credential encryption/decryption that weaken security
|
||||
- OAuth state/CSRF token handling that bypasses validation
|
||||
- Webhook requests that don't sanitize auth cookies
|
||||
- External secrets provider integrations with insecure configurations
|
||||
- Credential access not respecting scope boundaries (instance/project/user)
|
||||
- Data fetched via credentials being exposed to unauthorized user groups
|
||||
|
||||
An error message that interpolates a caught error from an integration is a
|
||||
common way credential material reaches a persisted status field or an API
|
||||
response. Sanitize before recording.
|
||||
32
.agents/review-rules/security/data-and-infrastructure.md
Normal file
32
.agents/review-rules/security/data-and-infrastructure.md
Normal file
|
|
@ -0,0 +1,32 @@
|
|||
# Filesystem, database, network, and audit trails
|
||||
|
||||
Applies to: all backend packages.
|
||||
|
||||
## Filesystem
|
||||
|
||||
- Weakened file access restriction enforcement
|
||||
- File operations bypassing allowlist/blocklist patterns
|
||||
- Access to n8n internal directories
|
||||
- Unsanitized user input flowing into a file path
|
||||
|
||||
## Database
|
||||
|
||||
- Missing indexes on frequently queried columns in migrations
|
||||
- Resource-intensive queries without pagination or limits
|
||||
- Missing encryption for sensitive fields beyond credentials
|
||||
- Raw SQL that bypasses TypeORM protections
|
||||
|
||||
## HTTP, webhooks, and network
|
||||
|
||||
- SSRF risk in user-controlled URLs
|
||||
- CORS allowing all origins
|
||||
- CSP or iframe sandbox changes that weaken protections
|
||||
- Rate limiting changes that reduce protection
|
||||
- Disabled TLS certificate validation (`rejectUnauthorized: false`)
|
||||
- Bearer tokens forwarded across a redirect to a different host
|
||||
|
||||
## Audit logging
|
||||
|
||||
- Missing logging for security-relevant events
|
||||
- Sensitive data in logs or error messages
|
||||
- Incomplete audit trails for authentication and authorization events
|
||||
77
.agents/review-rules/security/node-input-safety.md
Normal file
77
.agents/review-rules/security/node-input-safety.md
Normal file
|
|
@ -0,0 +1,77 @@
|
|||
# Node input safety
|
||||
|
||||
Applies to: `packages/nodes-base`, `packages/@n8n/nodes-langchain`. Skip this
|
||||
file for other packages.
|
||||
|
||||
Node code receives workflow-author-controlled input, mainly via
|
||||
`this.getNodeParameter(...)` and incoming `item.json` keys.
|
||||
|
||||
## Prototype pollution via node parameters
|
||||
|
||||
A value derived from user-controlled input must never be used as a computed
|
||||
object key in an **assignment** to a plain object. A workflow author can set it
|
||||
to `__proto__`, `constructor`, or `prototype`, polluting the prototype chain.
|
||||
|
||||
ESLint does not cover this — `no-prototype-builtins` is only a warning in these
|
||||
packages, and `pnpm lint` runs with `--quiet`.
|
||||
|
||||
Flag NEW code where a value tracing back to `this.getNodeParameter(...)`
|
||||
(directly, via a variable, via a `.map`/`.reduce`/`.forEach` callback param, or
|
||||
via a `getNodeParam` function passed into a helper such as `createTableStruct`)
|
||||
is used as a computed key to BUILD A NESTED OR CONTAINER STRUCTURE on a plain
|
||||
object (an `IDataObject` or object literal — not a `Map`, not
|
||||
`Object.create(null)`):
|
||||
|
||||
1. Container creation: `obj[key] = {}` or `obj[key] = []`
|
||||
2. Nested write where the untrusted value is a key: `obj[k1][k2] = value`
|
||||
3. The same via `obj[key] ??= {}` / `obj[key] ||= []`
|
||||
|
||||
This nested/container shape is what actually pollutes `Object.prototype` — the
|
||||
dangerous pattern is typically a shared accumulator.
|
||||
|
||||
Do NOT flag:
|
||||
|
||||
- Reads: `const x = obj[key]`, `if (obj[key] === undefined)` — reads don't pollute
|
||||
- Single-level writes of a concrete (non-object) value, e.g.
|
||||
`item.json[field] = value`, `item.binary[prop] = data`, `body[target] = value`.
|
||||
Assigning a primitive to `__proto__` is a no-op, and setting one property on a
|
||||
fresh per-item object is not prototype pollution. This is the overwhelmingly
|
||||
common, benign case in nodes — flagging it is noise.
|
||||
- Keys that are string/number literals, or validated by `isSafeObjectProperty(key)`
|
||||
- Writes routed through `setSafeObjectProperty(...)`
|
||||
- Targets that are a `Map` or created with `Object.create(null)`
|
||||
|
||||
Violation:
|
||||
|
||||
```typescript
|
||||
const table = this.getNodeParameter('table', i) as string;
|
||||
const key = this.getNodeParameter('deleteKey', i) as string;
|
||||
if (acc[table] === undefined) acc[table] = {}; // acc['__proto__'] = {}
|
||||
if (acc[table][key] === undefined) acc[table][key] = [];
|
||||
```
|
||||
|
||||
Allowed:
|
||||
|
||||
```typescript
|
||||
import { setSafeObjectProperty, isSafeObjectProperty } from 'n8n-workflow';
|
||||
|
||||
if (isSafeObjectProperty(table) && acc[table] === undefined) {
|
||||
setSafeObjectProperty(acc, table, {});
|
||||
}
|
||||
// or: const acc = new Map<string, ...>();
|
||||
```
|
||||
|
||||
Prefer `setSafeObjectProperty` / `isSafeObjectProperty` from `n8n-workflow`. See
|
||||
`nodes/Google/GSuiteAdmin/GSuiteAdmin.node.ts` and
|
||||
`nodes/HttpRequest/V3/HttpRequestV3.node.ts` for reference usage.
|
||||
|
||||
## Injection through node parameters
|
||||
|
||||
Flag NEW code where a node parameter reaches a sink without escaping or
|
||||
parameterisation:
|
||||
|
||||
- SQL query fields built by string concatenation or expression interpolation
|
||||
instead of bound parameters
|
||||
- Command execution with unsanitized shell arguments
|
||||
- File operations where a parameter reaches a path without traversal checks
|
||||
- Community package names accepted without validation
|
||||
2
.github/WORKFLOWS.md
vendored
2
.github/WORKFLOWS.md
vendored
|
|
@ -432,6 +432,7 @@ Push to master/1.x
|
|||
| Monday 00:00 | `util-update-node-popularity.yml` | Node usage stats |
|
||||
| Monday 02:00 | `test-e2e-coverage-weekly.yml` | Weekly E2E coverage |
|
||||
| Saturday 22:00 | `test-evals-ai.yml` | AI workflow evals |
|
||||
| 1st of month 04:00 | `util-refresh-cubic-schema.yml` | Refresh vendored cubic schema |
|
||||
|
||||
---
|
||||
|
||||
|
|
@ -553,6 +554,7 @@ Scripts in `.github/scripts/`:
|
|||
| `validate-docs-links.js`| Check doc URLs | `util-check-docs-urls.yml`|
|
||||
| `send-build-stats.mjs` | Build telemetry | `setup-nodejs` action |
|
||||
| `db-test-matrix.mjs` | DB test matrix from `postgres-versions.json` | `ci-pull-requests.yml` |
|
||||
| `quality/check-cubic-config.mjs` | Validate `cubic.yaml` against the vendored cubic schema; enforce its silent agent/character limits. `--refresh` re-pulls the schema | `test-workflow-scripts-reusable.yml`, `util-refresh-cubic-schema.yml` |
|
||||
| `probe-registry.mjs` | Registry path throughput probe (temporary) | `util-probe-registry.yml` |
|
||||
|
||||
### Branch Replay Scripts
|
||||
|
|
|
|||
4
.github/scripts/package.json
vendored
4
.github/scripts/package.json
vendored
|
|
@ -17,16 +17,18 @@
|
|||
"@actions/github": "9.0.0",
|
||||
"@cyclonedx/cdxgen": "12.4.0",
|
||||
"@octokit/core": "7.0.6",
|
||||
"ajv": "8.20.0",
|
||||
"conventional-changelog": "7.2.0",
|
||||
"debug": "4.4.3",
|
||||
"json-with-bigint": "3.5.11",
|
||||
"glob": "13.0.6",
|
||||
"json-with-bigint": "3.5.11",
|
||||
"minimatch": "10.2.4",
|
||||
"semver": "7.7.4",
|
||||
"tempfile": "6.0.1",
|
||||
"yaml": "^2.8.3"
|
||||
},
|
||||
"devDependencies": {
|
||||
"@types/node": "^26.3.0",
|
||||
"conventional-changelog-angular": "8.3.0"
|
||||
}
|
||||
}
|
||||
|
|
|
|||
18
.github/scripts/pnpm-lock.yaml
vendored
18
.github/scripts/pnpm-lock.yaml
vendored
|
|
@ -17,6 +17,9 @@ importers:
|
|||
'@octokit/core':
|
||||
specifier: 7.0.6
|
||||
version: 7.0.6
|
||||
ajv:
|
||||
specifier: 8.20.0
|
||||
version: 8.20.0
|
||||
conventional-changelog:
|
||||
specifier: 7.2.0
|
||||
version: 7.2.0(conventional-commits-filter@5.0.0)
|
||||
|
|
@ -42,6 +45,9 @@ importers:
|
|||
specifier: ^2.8.3
|
||||
version: 2.9.0
|
||||
devDependencies:
|
||||
'@types/node':
|
||||
specifier: ^26.3.0
|
||||
version: 26.3.0
|
||||
conventional-changelog-angular:
|
||||
specifier: 8.3.0
|
||||
version: 8.3.0
|
||||
|
|
@ -323,6 +329,9 @@ packages:
|
|||
'@types/http-cache-semantics@4.2.0':
|
||||
resolution: {integrity: sha512-L3LgimLHXtGkWikKnsPg0/VFx9OGZaC+eN1u4r+OB1XRqH3meBIAVC2zr1WdMH+RHmnRkqliQAOHNJ/E0j/e0Q==}
|
||||
|
||||
'@types/node@26.3.0':
|
||||
resolution: {integrity: sha512-L3fgrnchriRC2ExBflb8j4uZZURHZfQsmQeyVzhjcHW4kkwVyo8/0h1B2MVzMTrYUJYu6G7EWs14hW/L9putqw==}
|
||||
|
||||
'@types/normalize-package-data@2.4.4':
|
||||
resolution: {integrity: sha512-37i+OaWTh9qeK4LSHPsyRC7NahnGotNuZvjLSgcPzblpHB3rrCJxAOgI5gCdKm7coonsaX1Of0ILiTcnZjbfxA==}
|
||||
|
||||
|
|
@ -1072,6 +1081,9 @@ packages:
|
|||
engines: {node: '>=0.8.0'}
|
||||
hasBin: true
|
||||
|
||||
undici-types@8.3.0:
|
||||
resolution: {integrity: sha512-j375ScV60dom+YkPFIfTLcOiPxkN/buHz5GobjLhixFuANaNs3C9l4GmrWqejgXWJ7BbJcFYpTEUkS1Ge8bpZQ==}
|
||||
|
||||
undici@6.28.0:
|
||||
resolution: {integrity: sha512-LIY910g9TI13YS95lrMFrs8Rm/u/irgHeTWoKCoteeJ04CUJ92eEfj0rVn+7VKMPBpUPiUoBKfhNyLI23EE/KA==}
|
||||
engines: {node: '>=18.17'}
|
||||
|
|
@ -1523,6 +1535,10 @@ snapshots:
|
|||
|
||||
'@types/http-cache-semantics@4.2.0': {}
|
||||
|
||||
'@types/node@26.3.0':
|
||||
dependencies:
|
||||
undici-types: 8.3.0
|
||||
|
||||
'@types/normalize-package-data@2.4.4': {}
|
||||
|
||||
ajv-formats@3.0.1(ajv@8.20.0):
|
||||
|
|
@ -2324,6 +2340,8 @@ snapshots:
|
|||
uglify-js@3.19.3:
|
||||
optional: true
|
||||
|
||||
undici-types@8.3.0: {}
|
||||
|
||||
undici@6.28.0: {}
|
||||
|
||||
undici@7.29.0: {}
|
||||
|
|
|
|||
232
.github/scripts/quality/check-cubic-config.mjs
vendored
Normal file
232
.github/scripts/quality/check-cubic-config.mjs
vendored
Normal file
|
|
@ -0,0 +1,232 @@
|
|||
/**
|
||||
* Validates `cubic.yaml` against cubic's published schema and the limits it
|
||||
* enforces silently.
|
||||
*
|
||||
* cubic drops custom rules past the agent cap and truncates any rule past the
|
||||
* character ceiling without reporting either, and it never resolves a repo path
|
||||
* mentioned in prose — only `file_paths` entries. A rule that trips any of these
|
||||
* simply stops running, which is invisible until someone counts review comments.
|
||||
*
|
||||
* The schema is vendored rather than fetched so the check has no network
|
||||
* dependency. `--refresh` pulls the current copy from cubic.dev and exits without
|
||||
* validating — a refreshed schema that rejects the config should surface as a red
|
||||
* check on the refresh PR, not as a failure that stops the PR being opened.
|
||||
*
|
||||
* Exit codes:
|
||||
* 0 – config is valid
|
||||
* 1 – config has at least one violation
|
||||
*/
|
||||
|
||||
import { readdirSync, readFileSync, writeFileSync } from 'node:fs';
|
||||
import { dirname, join, relative, resolve } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import Ajv from 'ajv/dist/2020.js';
|
||||
import { parse } from 'yaml';
|
||||
|
||||
/** https://docs.cubic.dev/ai-review/custom-agents — only the first N agents take effect. */
|
||||
export const MAX_CUBIC_AGENTS = 5;
|
||||
|
||||
/** Description plus linked file contents; characters past this are dropped from the prompt. */
|
||||
export const MAX_RULE_CHARS = 10_000;
|
||||
|
||||
/** Fraction of the ceiling at which a rule is reported as close to silent truncation. */
|
||||
export const WARN_RATIO = 0.8;
|
||||
|
||||
/** Every markdown file here must be linked by some agent, or it silently does nothing. */
|
||||
export const RULES_DIR = '.agents/review-rules';
|
||||
|
||||
/** Vendored copy of the schema the `# yaml-language-server:` directive points at. */
|
||||
export const SCHEMA_PATH = '.github/scripts/quality/cubic-config.schema.json';
|
||||
|
||||
export const SCHEMA_URL = 'https://www.cubic.dev/schema/cubic-repository-config.schema.json';
|
||||
|
||||
const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '../../..');
|
||||
|
||||
/**
|
||||
* Characters, not bytes — cubic's ceiling is a character count, and a byte count
|
||||
* overstates it for any non-ASCII content (an em dash is 3 bytes, one character).
|
||||
* `.length` counts UTF-16 code units, matching how the description is measured.
|
||||
*
|
||||
* @param {string} path - repo-relative
|
||||
* @returns {number} character count, or -1 when the path does not resolve
|
||||
*/
|
||||
export function fileCharacters(path) {
|
||||
try {
|
||||
return readFileSync(join(REPO_ROOT, path), 'utf8').length;
|
||||
} catch {
|
||||
return -1;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Validate against cubic's own schema. Catches what the hand-written checks below
|
||||
* cannot: a mistyped key inside `reviews` / `pr_descriptions` / `issues` (all
|
||||
* `additionalProperties: false`), a bad enum value, a wrong type.
|
||||
*
|
||||
* @param {unknown} config
|
||||
* @param {object} schema
|
||||
* @returns {string[]}
|
||||
*/
|
||||
export function schemaErrors(config, schema) {
|
||||
const ajv = new Ajv({ allErrors: true, strict: false });
|
||||
const validate = ajv.compile(schema);
|
||||
if (validate(config)) return [];
|
||||
|
||||
return (validate.errors ?? []).map((error) => {
|
||||
const path = error.instancePath || '/';
|
||||
const { allowedValues, additionalProperty } = error.params ?? {};
|
||||
|
||||
if (additionalProperty) {
|
||||
return `${path} has an unknown key \`${additionalProperty}\`.`;
|
||||
}
|
||||
|
||||
const allowed = allowedValues ? ` (allowed: ${allowedValues.join(', ')})` : '';
|
||||
return `${path} ${error.message}${allowed}`;
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Markdown rule files on disk, repo-relative, excluding the README.
|
||||
*
|
||||
* @returns {string[]}
|
||||
*/
|
||||
function ruleFiles() {
|
||||
try {
|
||||
return readdirSync(join(REPO_ROOT, RULES_DIR), { recursive: true, withFileTypes: true })
|
||||
.filter((entry) => entry.isFile() && entry.name.endsWith('.md') && entry.name !== 'README.md')
|
||||
.map((entry) => relative(REPO_ROOT, join(entry.parentPath, entry.name)))
|
||||
.sort();
|
||||
} catch {
|
||||
return [];
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* @param {any} config - parsed cubic.yaml
|
||||
* @param {(path: string) => number} charsIn - characters in a linked file, -1 if missing
|
||||
* @param {string[]} [onDisk] - rule files that must each be linked by some agent
|
||||
* @returns {{ violations: string[], warnings: string[], ruleLengths: Record<string, number> }}
|
||||
*/
|
||||
export function checkConfig(config, charsIn, onDisk = []) {
|
||||
const violations = [];
|
||||
/** @type { string[] } */
|
||||
const warnings = [];
|
||||
/** @type { Record<string, number> } */
|
||||
const ruleLengths = {}
|
||||
const linked = new Set();
|
||||
|
||||
if (config?.version !== 1) {
|
||||
violations.push(`\`version\` must be 1, found ${JSON.stringify(config?.version)}.`);
|
||||
}
|
||||
|
||||
const rules = config?.reviews?.custom_rules ?? [];
|
||||
if (!Array.isArray(rules)) {
|
||||
violations.push('`reviews.custom_rules` must be a list.');
|
||||
return { violations, warnings, ruleLengths };
|
||||
}
|
||||
|
||||
if (rules.length > MAX_CUBIC_AGENTS) {
|
||||
const dropped = rules.slice(MAX_CUBIC_AGENTS).map((rule) => rule?.name ?? '(unnamed)');
|
||||
violations.push(
|
||||
`${rules.length} custom rules defined but only the first ${MAX_CUBIC_AGENTS} take effect. ` +
|
||||
`These never run: ${dropped.join(', ')}. Merge related rules instead of appending.`,
|
||||
);
|
||||
}
|
||||
|
||||
rules.forEach((rule, index) => {
|
||||
const label = rule?.name ? `"${rule.name}"` : `rule #${index + 1}`;
|
||||
|
||||
if (!rule?.name) {
|
||||
violations.push(`${label} has no \`name\`.`);
|
||||
}
|
||||
|
||||
const description = rule?.description ?? '';
|
||||
const filePaths = rule?.file_paths ?? [];
|
||||
|
||||
if (!description && filePaths.length === 0) {
|
||||
violations.push(`${label} needs a \`description\`, \`file_paths\`, or both.`);
|
||||
}
|
||||
|
||||
let total = description.length;
|
||||
for (const path of filePaths) {
|
||||
linked.add(path);
|
||||
const chars = charsIn(path);
|
||||
if (chars < 0) {
|
||||
violations.push(`${label} links \`${path}\`, which does not exist.`);
|
||||
continue;
|
||||
}
|
||||
total += chars;
|
||||
}
|
||||
|
||||
if (total > MAX_RULE_CHARS) {
|
||||
violations.push(
|
||||
`${label} is ${total.toLocaleString()} characters; everything past ` +
|
||||
`${MAX_RULE_CHARS.toLocaleString()} is dropped from the review prompt.`,
|
||||
);
|
||||
} else if (total > MAX_RULE_CHARS * WARN_RATIO) {
|
||||
warnings.push(
|
||||
`${label} is at ${Math.round((total / MAX_RULE_CHARS) * 100)}% of the ` +
|
||||
`${MAX_RULE_CHARS.toLocaleString()}-character ceiling. Trim it before adding more.`,
|
||||
);
|
||||
}
|
||||
|
||||
ruleLengths[label] = total
|
||||
});
|
||||
|
||||
for (const path of onDisk) {
|
||||
if (!linked.has(path)) {
|
||||
violations.push(`\`${path}\` is not linked by any agent, so it is never applied.`);
|
||||
}
|
||||
}
|
||||
|
||||
return { violations, warnings, ruleLengths };
|
||||
}
|
||||
|
||||
async function refreshSchema() {
|
||||
const response = await fetch(SCHEMA_URL);
|
||||
if (!response.ok) {
|
||||
console.error(`Could not fetch ${SCHEMA_URL}: HTTP ${response.status}`);
|
||||
process.exit(1);
|
||||
}
|
||||
const schema = await response.json();
|
||||
writeFileSync(join(REPO_ROOT, SCHEMA_PATH), `${JSON.stringify(schema, null, '\t')}\n`);
|
||||
console.log(`Refreshed ${SCHEMA_PATH} from ${SCHEMA_URL}.`);
|
||||
}
|
||||
|
||||
async function main() {
|
||||
if (process.argv.includes('--refresh')) {
|
||||
await refreshSchema();
|
||||
return;
|
||||
}
|
||||
|
||||
const config = parse(readFileSync(join(REPO_ROOT, 'cubic.yaml'), 'utf8'));
|
||||
const schema = JSON.parse(readFileSync(join(REPO_ROOT, SCHEMA_PATH), 'utf8'));
|
||||
|
||||
const { violations, warnings, ruleLengths } = checkConfig(config, fileCharacters, ruleFiles());
|
||||
violations.unshift(...schemaErrors(config, schema));
|
||||
|
||||
console.log("Rule sizes:")
|
||||
for (const [label, ruleLength] of Object.entries(ruleLengths)) {
|
||||
console.log(` ${label}: ${ruleLength} characters (${Math.floor(ruleLength / MAX_RULE_CHARS * 100)}%)`);
|
||||
}
|
||||
|
||||
for (const warning of warnings) {
|
||||
console.log(`::warning file=cubic.yaml::${warning}`);
|
||||
}
|
||||
|
||||
if (violations.length === 0) {
|
||||
console.log(
|
||||
`cubic.yaml is valid (${config.reviews?.custom_rules?.length ?? 0}/${MAX_CUBIC_AGENTS} agents).`,
|
||||
);
|
||||
return;
|
||||
}
|
||||
|
||||
for (const violation of violations) {
|
||||
console.log(`::error file=cubic.yaml::${violation}`);
|
||||
}
|
||||
process.exit(1);
|
||||
}
|
||||
|
||||
if (import.meta.url === `file://${process.argv[1]}`) {
|
||||
await main();
|
||||
}
|
||||
237
.github/scripts/quality/check-cubic-config.test.mjs
vendored
Normal file
237
.github/scripts/quality/check-cubic-config.test.mjs
vendored
Normal file
|
|
@ -0,0 +1,237 @@
|
|||
import assert from 'node:assert/strict';
|
||||
import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { join, relative } from 'node:path';
|
||||
import { after, before, describe, it } from 'node:test';
|
||||
|
||||
/**
|
||||
* Run with:
|
||||
* node --test .github/scripts/quality/check-cubic-config.test.mjs
|
||||
*/
|
||||
|
||||
let checkConfig, fileCharacters, schemaErrors, MAX_CUBIC_AGENTS, MAX_RULE_CHARS, WARN_RATIO;
|
||||
let schema;
|
||||
before(async () => {
|
||||
({ checkConfig, fileCharacters, schemaErrors, MAX_CUBIC_AGENTS, MAX_RULE_CHARS, WARN_RATIO } =
|
||||
await import('./check-cubic-config.mjs'));
|
||||
schema = JSON.parse(readFileSync(new URL('./cubic-config.schema.json', import.meta.url), 'utf8'));
|
||||
});
|
||||
|
||||
/** Violations only, for the cases that assert nothing about warnings. */
|
||||
const violationsOf = (...args) => checkConfig(...args).violations;
|
||||
|
||||
/** @param {Array<object>} rules */
|
||||
const config = (rules) => ({ version: 1, reviews: { custom_rules: rules } });
|
||||
|
||||
/** @param {string} name */
|
||||
const rule = (name) => ({ name, description: 'x' });
|
||||
|
||||
const noFiles = () => -1;
|
||||
const emptyFiles = () => 0;
|
||||
|
||||
describe('limits', () => {
|
||||
it('caps agents at 5', () => {
|
||||
assert.equal(MAX_CUBIC_AGENTS, 5);
|
||||
});
|
||||
|
||||
it('caps rule size at 10000', () => {
|
||||
assert.equal(MAX_RULE_CHARS, 10_000);
|
||||
});
|
||||
|
||||
it('warns at 80% of the ceiling', () => {
|
||||
assert.equal(WARN_RATIO, 0.8);
|
||||
});
|
||||
});
|
||||
|
||||
describe('checkConfig', () => {
|
||||
it('accepts a config at the agent cap', () => {
|
||||
const rules = Array.from({ length: MAX_CUBIC_AGENTS }, (_, i) => rule(`r${i}`));
|
||||
assert.deepEqual(violationsOf(config(rules), emptyFiles), []);
|
||||
});
|
||||
|
||||
it('names every rule that will never run when the cap is exceeded', () => {
|
||||
const rules = Array.from({ length: MAX_CUBIC_AGENTS + 2 }, (_, i) => rule(`r${i}`));
|
||||
const [violation, ...rest] = violationsOf(config(rules), emptyFiles);
|
||||
|
||||
assert.deepEqual(rest, []);
|
||||
assert.match(violation, /only the first 5 take effect/);
|
||||
assert.match(violation, /r5, r6/);
|
||||
});
|
||||
|
||||
it('flags a linked file that does not exist', () => {
|
||||
const rules = [{ name: 'Frontend', description: '', file_paths: ['.agents/gone.md'] }];
|
||||
const violations = violationsOf(config(rules), noFiles);
|
||||
|
||||
assert.equal(violations.length, 1);
|
||||
assert.match(violations[0], /"Frontend" links `\.agents\/gone\.md`, which does not exist/);
|
||||
});
|
||||
|
||||
it('counts linked file length toward the character ceiling', () => {
|
||||
const rules = [{ name: 'Big', description: 'a'.repeat(9_000), file_paths: ['doc.md'] }];
|
||||
|
||||
assert.deepEqual(
|
||||
violationsOf(config(rules), () => 500),
|
||||
[],
|
||||
);
|
||||
|
||||
const violations = violationsOf(config(rules), () => 2_000);
|
||||
assert.equal(violations.length, 1);
|
||||
assert.match(violations[0], /"Big" is 11,000 characters/);
|
||||
});
|
||||
|
||||
it('measures a description in characters, not UTF-8 bytes', () => {
|
||||
// Each em dash is 3 bytes but one character. By bytes this is 12,000 and
|
||||
// would fail; by characters it is 4,000 and fits.
|
||||
const rules = [{ name: 'Dashes', description: '—'.repeat(4_000) }];
|
||||
|
||||
assert.deepEqual(violationsOf(config(rules), emptyFiles), []);
|
||||
});
|
||||
|
||||
it('flags a rule with neither a description nor linked files', () => {
|
||||
const violations = violationsOf(config([{ name: 'Empty' }]), emptyFiles);
|
||||
|
||||
assert.equal(violations.length, 1);
|
||||
assert.match(violations[0], /needs a `description`, `file_paths`, or both/);
|
||||
});
|
||||
|
||||
it('flags an unnamed rule', () => {
|
||||
const violations = violationsOf(config([{ description: 'x' }]), emptyFiles);
|
||||
|
||||
assert.equal(violations.length, 1);
|
||||
assert.match(violations[0], /rule #1 has no `name`/);
|
||||
});
|
||||
|
||||
it('flags a wrong schema version', () => {
|
||||
const violations = violationsOf({ version: 2, reviews: { custom_rules: [] } }, emptyFiles);
|
||||
|
||||
assert.equal(violations.length, 1);
|
||||
assert.match(violations[0], /`version` must be 1, found 2/);
|
||||
});
|
||||
|
||||
it('accepts a config with no custom rules at all', () => {
|
||||
assert.deepEqual(violationsOf({ version: 1 }, emptyFiles), []);
|
||||
});
|
||||
|
||||
it('flags a rule file that no agent links', () => {
|
||||
const rules = [{ name: 'Backend', description: '', file_paths: ['rules/linked.md'] }];
|
||||
const onDisk = ['rules/linked.md', 'rules/orphan.md'];
|
||||
const { violations } = checkConfig(config(rules), emptyFiles, onDisk);
|
||||
|
||||
assert.equal(violations.length, 1);
|
||||
assert.match(violations[0], /`rules\/orphan\.md` is not linked by any agent/);
|
||||
});
|
||||
|
||||
it('accepts rule files that are all linked', () => {
|
||||
const rules = [
|
||||
{ name: 'A', description: '', file_paths: ['rules/a.md'] },
|
||||
{ name: 'B', description: '', file_paths: ['rules/b.md'] },
|
||||
];
|
||||
const { violations } = checkConfig(config(rules), emptyFiles, ['rules/a.md', 'rules/b.md']);
|
||||
|
||||
assert.deepEqual(violations, []);
|
||||
});
|
||||
});
|
||||
|
||||
describe('ceiling warnings', () => {
|
||||
it('warns without failing once a rule passes 80%', () => {
|
||||
const rules = [{ name: 'Security', description: 'a'.repeat(8_500) }];
|
||||
const { violations, warnings } = checkConfig(config(rules), emptyFiles);
|
||||
|
||||
assert.deepEqual(violations, []);
|
||||
assert.equal(warnings.length, 1);
|
||||
assert.match(warnings[0], /"Security" is at 85% of the 10,000-character ceiling/);
|
||||
});
|
||||
|
||||
it('stays quiet below the warning threshold', () => {
|
||||
const rules = [{ name: 'Security', description: 'a'.repeat(7_000) }];
|
||||
const { violations, warnings } = checkConfig(config(rules), emptyFiles);
|
||||
|
||||
assert.deepEqual(violations, []);
|
||||
assert.deepEqual(warnings, []);
|
||||
});
|
||||
|
||||
it('reports a violation rather than a warning once over the ceiling', () => {
|
||||
const rules = [{ name: 'Security', description: 'a'.repeat(10_001) }];
|
||||
const { violations, warnings } = checkConfig(config(rules), emptyFiles);
|
||||
|
||||
assert.equal(violations.length, 1);
|
||||
assert.deepEqual(warnings, []);
|
||||
});
|
||||
});
|
||||
|
||||
describe('schemaErrors', () => {
|
||||
/** @param {object} reviews */
|
||||
const cfg = (reviews) => ({ version: 1, reviews });
|
||||
|
||||
it('accepts the settings we actually use', () => {
|
||||
const errors = schemaErrors(
|
||||
cfg({ enabled: true, sensitivity: 'medium', incremental_commits: true, check_drafts: true }),
|
||||
schema,
|
||||
);
|
||||
assert.deepEqual(errors, []);
|
||||
});
|
||||
|
||||
it('rejects a sensitivity outside the enum and lists the allowed values', () => {
|
||||
const [error, ...rest] = schemaErrors(cfg({ sensitivity: 'strict' }), schema);
|
||||
|
||||
assert.deepEqual(rest, []);
|
||||
assert.match(error, /\/reviews\/sensitivity must be equal to one of the allowed values/);
|
||||
assert.match(error, /allowed: low, medium, high/);
|
||||
});
|
||||
|
||||
it('names a mistyped key rather than saying "additional properties"', () => {
|
||||
const [error, ...rest] = schemaErrors(cfg({ check_draft: true }), schema);
|
||||
|
||||
assert.deepEqual(rest, []);
|
||||
assert.equal(error, '/reviews has an unknown key `check_draft`.');
|
||||
});
|
||||
|
||||
it('rejects a wrong type', () => {
|
||||
const [error] = schemaErrors(cfg({ incremental_commits: 'sometimes' }), schema);
|
||||
assert.match(error, /\/reviews\/incremental_commits must be boolean/);
|
||||
});
|
||||
|
||||
it('rejects a per-rule field cubic does not support', () => {
|
||||
const config = cfg({ custom_rules: [{ name: 'A', description: 'x', severity: 'high' }] });
|
||||
const [error] = schemaErrors(config, schema);
|
||||
|
||||
assert.equal(error, '/reviews/custom_rules/0 has an unknown key `severity`.');
|
||||
});
|
||||
|
||||
it('requires a version', () => {
|
||||
const errors = schemaErrors({ reviews: {} }, schema);
|
||||
assert.ok(errors.some((e) => e.includes('version')));
|
||||
});
|
||||
});
|
||||
|
||||
describe('fileCharacters', () => {
|
||||
const REPO_ROOT = new URL('../../..', import.meta.url).pathname;
|
||||
let dir;
|
||||
|
||||
before(() => {
|
||||
dir = mkdtempSync(join(tmpdir(), 'cubic-chars-'));
|
||||
});
|
||||
after(() => rmSync(dir, { recursive: true, force: true }));
|
||||
|
||||
/** @param {string} name @param {string} content */
|
||||
const write = (name, content) => {
|
||||
const abs = join(dir, name);
|
||||
writeFileSync(abs, content, 'utf8');
|
||||
return relative(REPO_ROOT, abs);
|
||||
};
|
||||
|
||||
it('counts characters, not UTF-8 bytes', () => {
|
||||
// 100 em dashes: 300 bytes, 100 characters. statSync().size would say 300.
|
||||
const path = write('dashes.md', '—'.repeat(100));
|
||||
assert.equal(fileCharacters(path), 100);
|
||||
});
|
||||
|
||||
it('counts ASCII unchanged, where bytes and characters agree', () => {
|
||||
const path = write('ascii.md', 'a'.repeat(100));
|
||||
assert.equal(fileCharacters(path), 100);
|
||||
});
|
||||
|
||||
it('returns -1 for a path that does not resolve', () => {
|
||||
assert.equal(fileCharacters('.agents/review-rules/does-not-exist.md'), -1);
|
||||
});
|
||||
});
|
||||
348
.github/scripts/quality/cubic-config.schema.json
vendored
Normal file
348
.github/scripts/quality/cubic-config.schema.json
vendored
Normal file
|
|
@ -0,0 +1,348 @@
|
|||
{
|
||||
"$schema": "https://json-schema.org/draft/2020-12/schema",
|
||||
"$id": "https://cubic.dev/schema/cubic-repository-config.schema.json",
|
||||
"title": "Cubic Repository Configuration",
|
||||
"description": "JSON schema for cubic.yaml (and the legacy .cubic.yaml) so editors can validate and autocomplete repository settings.",
|
||||
"type": "object",
|
||||
"additionalProperties": true,
|
||||
"required": [
|
||||
"version"
|
||||
],
|
||||
"properties": {
|
||||
"version": {
|
||||
"const": 1,
|
||||
"description": "Schema version. Must always be 1."
|
||||
},
|
||||
"reviews": {
|
||||
"type": "object",
|
||||
"description": "AI review behavior and optional YAML-defined custom agents.",
|
||||
"additionalProperties": false,
|
||||
"properties": {
|
||||
"enabled": {
|
||||
"type": "boolean",
|
||||
"description": "Master toggle for AI reviews on this repository."
|
||||
},
|
||||
"sensitivity": {
|
||||
"type": "string",
|
||||
"enum": [
|
||||
"low",
|
||||
"medium",
|
||||
"high"
|
||||
],
|
||||
"description": "Controls how strictly Cubic flags issues."
|
||||
},
|
||||
"incremental_commits": {
|
||||
"type": "boolean",
|
||||
"default": true,
|
||||
"description": "If true, reviews new commits pushed to open PRs (only new issues are posted). If false, reviews only when the PR is first opened."
|
||||
},
|
||||
"check_drafts": {
|
||||
"type": "boolean",
|
||||
"default": false,
|
||||
"description": "If true, reviews draft PRs immediately when opened. If false, skips them."
|
||||
},
|
||||
"architecture_diagrams": {
|
||||
"type": "boolean",
|
||||
"default": false,
|
||||
"description": "If true, includes AI-generated architecture diagrams in review summaries."
|
||||
},
|
||||
"external_contributors_require_manual_review": {
|
||||
"type": "boolean",
|
||||
"default": false,
|
||||
"description": "If true, cubic skips automatic reviews for public-repository PRs from external contributors until a trusted installation member manually triggers a review."
|
||||
},
|
||||
"show_ai_feedback_buttons": {
|
||||
"type": "boolean",
|
||||
"default": false,
|
||||
"deprecated": true,
|
||||
"description": "Deprecated and ignored. Retained so existing cubic.yaml files continue to validate."
|
||||
},
|
||||
"resolve_threads_when_addressed": {
|
||||
"type": "boolean",
|
||||
"default": true,
|
||||
"description": "Automatically resolve GitHub review threads when cubic detects the issue has been addressed in a subsequent commit."
|
||||
},
|
||||
"merge_confidence_summary": {
|
||||
"type": "boolean",
|
||||
"default": false,
|
||||
"description": "If true, includes an AI-generated merge confidence summary on full reviews."
|
||||
},
|
||||
"auto_approve_behavior": {
|
||||
"type": "string",
|
||||
"enum": [
|
||||
"disabled",
|
||||
"shadow",
|
||||
"live"
|
||||
],
|
||||
"description": "Controls whether auto-approval is disabled, simulated in shadow mode, or submitted live to GitHub."
|
||||
},
|
||||
"auto_approve": {
|
||||
"type": "string",
|
||||
"enum": [
|
||||
"disabled",
|
||||
"always",
|
||||
"low_risk_only",
|
||||
"custom"
|
||||
],
|
||||
"description": "Auto-approve mode: disabled, always approve clean runs, use low-risk heuristics, or use a custom prompt."
|
||||
},
|
||||
"auto_approve_custom_prompt": {
|
||||
"type": "string",
|
||||
"description": "Custom criteria for auto-approval. Only used when auto_approve is custom."
|
||||
},
|
||||
"auto_approve_rules": {
|
||||
"$ref": "#/$defs/autoApproveRules"
|
||||
},
|
||||
"ultrareview": {
|
||||
"type": "string",
|
||||
"enum": [
|
||||
"disabled",
|
||||
"manual",
|
||||
"automatic"
|
||||
],
|
||||
"description": "Master switch for ultrareviews. disabled blocks all ultrareviews (manual and automatic), manual allows only manual triggers (the default), automatic also enables the automatic triggers."
|
||||
},
|
||||
"auto_ultrareview": {
|
||||
"type": "string",
|
||||
"enum": [
|
||||
"disabled",
|
||||
"high_risk_only",
|
||||
"custom"
|
||||
],
|
||||
"description": "Auto-trigger ultrareview mode: disabled, let cubic decide per PR using built-in high-risk criteria, or use a custom prompt."
|
||||
},
|
||||
"auto_ultrareview_custom_prompt": {
|
||||
"type": "string",
|
||||
"description": "Custom criteria describing which PRs warrant an ultrareview. Only used when auto_ultrareview is custom."
|
||||
},
|
||||
"auto_ultrareview_file_patterns": {
|
||||
"$ref": "#/$defs/globList",
|
||||
"description": "File or directory globs that always trigger an ultrareview. If any changed file in a PR matches, cubic runs an ultrareview regardless of the auto_ultrareview mode."
|
||||
},
|
||||
"custom_instructions": {
|
||||
"type": "string",
|
||||
"description": "Free-form reviewer guidance. Leading/trailing whitespace is trimmed."
|
||||
},
|
||||
"ignore": {
|
||||
"$ref": "#/$defs/ignore"
|
||||
},
|
||||
"custom_rules": {
|
||||
"type": "array",
|
||||
"description": "List of YAML-managed custom agents enforced before UI-defined agents.",
|
||||
"items": {
|
||||
"$ref": "#/$defs/customRule"
|
||||
}
|
||||
}
|
||||
}
|
||||
},
|
||||
"pr_descriptions": {
|
||||
"type": "object",
|
||||
"description": "Configuration for AI-authored pull request descriptions.",
|
||||
"additionalProperties": false,
|
||||
"properties": {
|
||||
"generate": {
|
||||
"type": "boolean",
|
||||
"description": "Enable AI-authored PR descriptions."
|
||||
},
|
||||
"instructions": {
|
||||
"type": "string",
|
||||
"description": "Extra guidance inserted into generated summaries."
|
||||
},
|
||||
"cubic_review_link": {
|
||||
"type": "boolean",
|
||||
"description": "Include a link to review the PR in cubic (eg www.cubic.dev/pr/owner/repo/123)."
|
||||
},
|
||||
"skip_if_author_description": {
|
||||
"type": "boolean",
|
||||
"description": "Skip AI description generation on new PRs when the author already wrote a substantive description. Empty or template-only descriptions still get an AI description."
|
||||
}
|
||||
}
|
||||
},
|
||||
"issues": {
|
||||
"type": "object",
|
||||
"description": "Issue and PR comment fix settings.",
|
||||
"additionalProperties": false,
|
||||
"properties": {
|
||||
"fix_with_cubic_buttons": {
|
||||
"type": "boolean",
|
||||
"description": "Toggle Fix with cubic buttons inside GitHub issues."
|
||||
},
|
||||
"pr_comment_fixes": {
|
||||
"type": "boolean",
|
||||
"description": "Allow cubic to make code changes when requested from PR comment threads."
|
||||
},
|
||||
"fix_commits_to_pr": {
|
||||
"type": "boolean",
|
||||
"description": "When true, fix commits are pushed directly to the PR branch instead of opening a new PR."
|
||||
},
|
||||
"auto_fix_sign_commits": {
|
||||
"type": "boolean",
|
||||
"default": false,
|
||||
"description": "When true, cubic signs the commits it pushes so GitHub marks them as verified. This applies only to the 'cubic' coding agent; the Cursor agent always signs its commits."
|
||||
},
|
||||
"coding_agent_provider": {
|
||||
"type": "string",
|
||||
"enum": [
|
||||
"cubic",
|
||||
"cursor_cloud_agent"
|
||||
],
|
||||
"description": "Which coding agent applies automatic fixes. 'cubic' uses cubic's built-in agent; 'cursor_cloud_agent' dispatches a Cursor cloud agent (requires Cursor configuration)."
|
||||
}
|
||||
}
|
||||
}
|
||||
},
|
||||
"$defs": {
|
||||
"globList": {
|
||||
"type": "array",
|
||||
"description": "List of glob patterns. Empty strings are ignored at runtime.",
|
||||
"items": {
|
||||
"type": "string",
|
||||
"minLength": 1
|
||||
}
|
||||
},
|
||||
"ignore": {
|
||||
"type": "object",
|
||||
"description": "Conditional ignore filters that skip AI reviews.",
|
||||
"additionalProperties": false,
|
||||
"properties": {
|
||||
"files": {
|
||||
"$ref": "#/$defs/globList",
|
||||
"description": "File globs to skip entirely."
|
||||
},
|
||||
"head_branches": {
|
||||
"$ref": "#/$defs/globList",
|
||||
"description": "Source branches that should not trigger reviews."
|
||||
},
|
||||
"base_branches": {
|
||||
"$ref": "#/$defs/globList",
|
||||
"description": "Target branches that should not trigger reviews."
|
||||
},
|
||||
"pr_labels": {
|
||||
"$ref": "#/$defs/globList",
|
||||
"description": "Labels that disable reviews when present."
|
||||
},
|
||||
"pr_titles": {
|
||||
"$ref": "#/$defs/globList",
|
||||
"description": "Wildcard matches applied to pull request titles."
|
||||
},
|
||||
"max_changed_lines": {
|
||||
"type": "integer",
|
||||
"minimum": 1,
|
||||
"maximum": 2147483647,
|
||||
"description": "Store the automatic-review threshold for reviewable added plus deleted lines. Positive integers up to 2,147,483,647 are preserved, but cubic applies an effective 50,000-line ceiling to every review. Manual triggers can bypass a lower configured threshold, but not the effective ceiling."
|
||||
}
|
||||
}
|
||||
},
|
||||
"autoApproveRules": {
|
||||
"type": "object",
|
||||
"description": "Rules that control whether auto-approval evaluation is allowed.",
|
||||
"additionalProperties": false,
|
||||
"properties": {
|
||||
"exclude": {
|
||||
"$ref": "#/$defs/globList",
|
||||
"description": "Changed files matching these globs are never eligible for auto-approval."
|
||||
},
|
||||
"only_files": {
|
||||
"$ref": "#/$defs/globList",
|
||||
"description": "Every changed path must match at least one of these globs for the pull request to be eligible for auto-approval. Renamed files must match on both their current and previous paths."
|
||||
},
|
||||
"exclude_external_contributors": {
|
||||
"type": "boolean",
|
||||
"default": false,
|
||||
"description": "If true, public-repository PRs from external contributors are reviewed but never auto-approved."
|
||||
},
|
||||
"exclude_authors": {
|
||||
"$ref": "#/$defs/globList",
|
||||
"description": "PRs from a matching author are never auto-approved. Logins match case-insensitively and brackets are literal, so `renovate[bot]` and `*[bot]` both work."
|
||||
},
|
||||
"only_authors": {
|
||||
"$ref": "#/$defs/globList",
|
||||
"description": "Only PRs from a matching author can be auto-approved."
|
||||
},
|
||||
"exclude_head_branches": {
|
||||
"$ref": "#/$defs/globList",
|
||||
"description": "PRs whose source branch matches are never auto-approved."
|
||||
},
|
||||
"only_head_branches": {
|
||||
"$ref": "#/$defs/globList",
|
||||
"description": "Only PRs whose source branch matches can be auto-approved."
|
||||
},
|
||||
"exclude_base_branches": {
|
||||
"$ref": "#/$defs/globList",
|
||||
"description": "PRs targeting a matching base branch are never auto-approved."
|
||||
},
|
||||
"only_base_branches": {
|
||||
"$ref": "#/$defs/globList",
|
||||
"description": "Only PRs targeting a matching base branch can be auto-approved."
|
||||
},
|
||||
"exclude_labels": {
|
||||
"$ref": "#/$defs/globList",
|
||||
"description": "PRs carrying a matching label are never auto-approved. Labels match exactly, case-insensitively."
|
||||
},
|
||||
"only_labels": {
|
||||
"$ref": "#/$defs/globList",
|
||||
"description": "Only PRs carrying a matching label can be auto-approved. Labels match exactly, case-insensitively."
|
||||
},
|
||||
"exclude_titles": {
|
||||
"$ref": "#/$defs/globList",
|
||||
"description": "PRs whose title matches are never auto-approved."
|
||||
},
|
||||
"only_titles": {
|
||||
"$ref": "#/$defs/globList",
|
||||
"description": "Only PRs whose title matches can be auto-approved."
|
||||
}
|
||||
}
|
||||
},
|
||||
"customRule": {
|
||||
"type": "object",
|
||||
"description": "Definition of a YAML-managed custom agent.",
|
||||
"additionalProperties": false,
|
||||
"required": [
|
||||
"name"
|
||||
],
|
||||
"anyOf": [
|
||||
{
|
||||
"required": [
|
||||
"description"
|
||||
]
|
||||
},
|
||||
{
|
||||
"required": [
|
||||
"file_paths"
|
||||
]
|
||||
}
|
||||
],
|
||||
"properties": {
|
||||
"name": {
|
||||
"type": "string",
|
||||
"minLength": 1,
|
||||
"description": "Rule title shown in the dashboard."
|
||||
},
|
||||
"description": {
|
||||
"type": "string",
|
||||
"minLength": 1,
|
||||
"description": "Explain what the rule enforces."
|
||||
},
|
||||
"file_paths": {
|
||||
"type": "array",
|
||||
"minItems": 1,
|
||||
"maxItems": 10,
|
||||
"description": "Ordered repo-relative instruction files. The first 10,000 characters of the description plus concatenated file text are used.",
|
||||
"items": {
|
||||
"type": "string",
|
||||
"minLength": 1,
|
||||
"maxLength": 500
|
||||
}
|
||||
},
|
||||
"include": {
|
||||
"$ref": "#/$defs/globList",
|
||||
"description": "Optional include globs. Defaults to all files."
|
||||
},
|
||||
"exclude": {
|
||||
"$ref": "#/$defs/globList",
|
||||
"description": "Optional exclude globs that override includes."
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
2
.github/workflows/ci-pull-requests.yml
vendored
2
.github/workflows/ci-pull-requests.yml
vendored
|
|
@ -117,6 +117,8 @@ jobs:
|
|||
.github/scripts/**
|
||||
scripts/licenses/**
|
||||
scripts/mutation-health/**
|
||||
cubic.yaml
|
||||
.agents/review-rules/**
|
||||
performance:
|
||||
packages/testing/performance/**
|
||||
packages/workflow/src/**
|
||||
|
|
|
|||
|
|
@ -36,3 +36,7 @@ jobs:
|
|||
- name: Run tests
|
||||
id: run-tests
|
||||
run: npm test --prefix=.github/scripts
|
||||
|
||||
- name: Check cubic.yaml
|
||||
if: ${{ !cancelled() }}
|
||||
run: node .github/scripts/quality/check-cubic-config.mjs
|
||||
|
|
|
|||
85
.github/workflows/util-refresh-cubic-schema.yml
vendored
Normal file
85
.github/workflows/util-refresh-cubic-schema.yml
vendored
Normal file
|
|
@ -0,0 +1,85 @@
|
|||
name: 'Util: Refresh cubic schema'
|
||||
|
||||
on:
|
||||
schedule:
|
||||
# 1st of the month at 04:00 UTC
|
||||
- cron: '0 4 1 * *'
|
||||
workflow_dispatch: # Allow manual trigger for testing
|
||||
|
||||
permissions:
|
||||
contents: read
|
||||
|
||||
jobs:
|
||||
refresh-schema:
|
||||
name: Refresh vendored cubic schema
|
||||
if: |
|
||||
github.event_name == 'workflow_dispatch' ||
|
||||
(github.event_name == 'schedule' && github.repository == 'n8n-io/n8n')
|
||||
runs-on: ubuntu-latest
|
||||
timeout-minutes: 10
|
||||
# Every write goes through the scoped app token below, so the job's own
|
||||
# GITHUB_TOKEN only needs to read for checkout.
|
||||
permissions:
|
||||
contents: read
|
||||
steps:
|
||||
- name: Generate GitHub App Token
|
||||
id: generate-token
|
||||
uses: actions/create-github-app-token@29824e69f54612133e76f7eaac726eef6c875baf # v2.2.1
|
||||
with:
|
||||
app-id: ${{ secrets.N8N_ASSISTANT_APP_ID }}
|
||||
private-key: ${{ secrets.N8N_ASSISTANT_PRIVATE_KEY }}
|
||||
# Scope the token to what create-pull-request does: push the branch,
|
||||
# open the PR (the label already exists, so no issues access needed).
|
||||
permission-contents: write
|
||||
permission-pull-requests: write
|
||||
|
||||
- name: Checkout repository
|
||||
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
|
||||
with:
|
||||
# create-pull-request unsets any persisted credential and configures
|
||||
# its own from `token`, so nothing here needs to survive the checkout.
|
||||
persist-credentials: false
|
||||
|
||||
- name: Setup Node.js
|
||||
uses: ./.github/actions/setup-nodejs
|
||||
with:
|
||||
build-command: ''
|
||||
install-command: pnpm install --frozen-lockfile --dir ./.github/scripts --ignore-workspace
|
||||
cache-dependency-path: .github/scripts/pnpm-lock.yaml
|
||||
|
||||
# Refreshes only. Validation runs on the PR this opens, so a schema change
|
||||
# that rejects the current cubic.yaml shows up as a red check to act on
|
||||
# rather than a failure here that hides the change.
|
||||
- name: Refresh vendored schema
|
||||
run: node .github/scripts/quality/check-cubic-config.mjs --refresh
|
||||
|
||||
- name: Create Pull Request
|
||||
uses: peter-evans/create-pull-request@c0f553fe549906ede9cf27b5156039d195d2ece0 # v8.1.0
|
||||
with:
|
||||
token: ${{ steps.generate-token.outputs.token }}
|
||||
# Stage only the schema. setup-nodejs can leave unrelated files dirty,
|
||||
# and this PR should never carry anything but the refreshed copy.
|
||||
add-paths: .github/scripts/quality/cubic-config.schema.json
|
||||
commit-message: 'chore: Refresh vendored cubic config schema (no-changelog)'
|
||||
labels: 'automation:scheduled-update'
|
||||
title: 'chore: Refresh vendored cubic config schema (no-changelog)'
|
||||
body: |
|
||||
cubic's published config schema has changed since the vendored copy was last updated.
|
||||
|
||||
The copy at `.github/scripts/quality/cubic-config.schema.json` is what
|
||||
`pnpm check:cubic-config` validates `cubic.yaml` against, so it is vendored
|
||||
to keep that check off the network.
|
||||
|
||||
**Review the diff for new options worth adopting** — new keys under `reviews`
|
||||
are how cubic ships features, and the vendored copy is the only place they
|
||||
become visible to us.
|
||||
|
||||
If the `Workflow scripts` check is red, the new schema rejects something in
|
||||
`cubic.yaml`; fix the config in this PR.
|
||||
|
||||
_Generated by the monthly cubic schema refresh workflow._
|
||||
branch: refresh-cubic-schema
|
||||
base: master
|
||||
delete-branch: true
|
||||
author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
||||
committer: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
||||
|
|
@ -237,7 +237,7 @@ a new import (or an inline `eslint-disable` of the rule) fails CI.
|
|||
- **All UI text must use i18n** - add translations to `@n8n/i18n` package
|
||||
- **Use CSS variables directly** - never hardcode spacing as px values
|
||||
- **data-testid must be a single value** (no spaces or multiple values)
|
||||
- Always use `design-system-rules` skill in reviews
|
||||
- Always use the `design-system` skill in reviews
|
||||
|
||||
### Testing Guidelines
|
||||
- **Always work from within the package directory** when running tests
|
||||
|
|
|
|||
467
cubic.yaml
467
cubic.yaml
|
|
@ -5,27 +5,57 @@
|
|||
# Place this file in your repository root to version-control your AI review settings.
|
||||
# Settings defined here take precedence over UI-configured settings.
|
||||
# See https://docs.cubic.dev/configure/cubic-yaml for documentation.
|
||||
#
|
||||
# Only the first 5 custom_rules take effect, and each one is truncated at 10,000
|
||||
# characters (description + linked file_paths). `pnpm check:cubic-config` enforces both.
|
||||
# Scope every rule with include/exclude so it is not evaluated against the whole monorepo.
|
||||
|
||||
version: 1
|
||||
reviews:
|
||||
enabled: true
|
||||
sensitivity: medium
|
||||
incremental_commits: true
|
||||
show_ai_feedback_buttons: false
|
||||
check_drafts: true
|
||||
ignore:
|
||||
# Mechanical PRs: content was already reviewed on master, or there is nothing
|
||||
# to review. Backports stay reviewed — their conflicts are resolved by hand.
|
||||
pr_labels:
|
||||
- automation:v3-sync
|
||||
- automation:release
|
||||
- automation:scheduled-update
|
||||
# Above this, cubic skips the automatic review; `@cubic-dev-ai review this`
|
||||
# still works. CI already caps PRs at 1000 lines, so crossing this is always
|
||||
# a deliberate exception. Makes explicit a limit cubic otherwise applies on
|
||||
# its own terms.
|
||||
max_changed_lines: 10000
|
||||
files:
|
||||
- CHANGELOG.md
|
||||
- pnpm-lock.yaml
|
||||
- '**/dist/**'
|
||||
- '**/__snapshots__/**'
|
||||
- '**/*.snap'
|
||||
- '**/*.generated.yml'
|
||||
- '**/*.generated.ts'
|
||||
custom_instructions: |-
|
||||
## Step 1: Fetch Current Guidelines
|
||||
## Scope
|
||||
|
||||
1. Fetch the current [CONTRIBUTING.md](https://github.com/n8n-io/n8n/blob/master/CONTRIBUTING.md) from the repository's main branch
|
||||
2. Navigate to the "Community PR Guidelines" section
|
||||
3. Use ONLY this live version - ignore any cached/embedded rules
|
||||
Review only the lines this PR adds or modifies. Each PR is meant to have a
|
||||
limited scope — do not report problems in surrounding code that already
|
||||
existed.
|
||||
|
||||
## Step 2: Review Process
|
||||
## Don't repeat the linter
|
||||
|
||||
Evaluate the PR against the rules in the fetched Community PR Guidelines.
|
||||
`pnpm lint` and `pnpm typecheck` run on every PR and already fail the build
|
||||
for a large set of n8n-specific rules (`@n8n/eslint-config`). Do not spend a
|
||||
comment on anything ESLint reports as an error — notably `ApplicationError`
|
||||
usage, `sleep`/`sleepWithAbort` helpers, `sleep` imported from
|
||||
`n8n-workflow`, `@n8n/typeorm` imports in `packages/cli` business logic,
|
||||
uncaught `JSON.parse`, `JSON.parse(JSON.stringify())`, skipped tests, and
|
||||
CSS custom-property naming. Report what static analysis cannot see.
|
||||
|
||||
## Step 3: Test Requirement Interpretation
|
||||
## Test coverage
|
||||
|
||||
BE REASONABLE when evaluating test coverage:
|
||||
BE REASONABLE when evaluating test coverage.
|
||||
|
||||
**PASS if:**
|
||||
|
||||
|
|
@ -41,334 +71,119 @@ reviews:
|
|||
|
||||
Approve if reasonably tested. Let humans handle edge cases.
|
||||
|
||||
## Step 4: Design System Style Rules
|
||||
## Community contributions
|
||||
|
||||
Follow `.claude/plugins/n8n/skills/design-system-rules/SKILL.md` for all CSS/SCSS/Vue style
|
||||
review guidance.
|
||||
|
||||
Enforcement level:
|
||||
- Strong warning: hard-coded visual values, legacy token usage, deprecated
|
||||
style/component surfaces
|
||||
- Soft warning: token-to-token substitutions (ask for intent to avoid
|
||||
accidental regressions)
|
||||
For PRs from outside the n8n organisation, the bar is set by the "Community
|
||||
PR Guidelines" section of `CONTRIBUTING.md`. The golden rule there: a
|
||||
contribution should be worth more to the project than the time it takes to
|
||||
review it.
|
||||
custom_rules:
|
||||
- name: Security Review
|
||||
- name: Security
|
||||
file_paths:
|
||||
- .agents/review-rules/security/code-execution-and-sandboxing.md
|
||||
- .agents/review-rules/security/credentials-and-secrets.md
|
||||
- .agents/review-rules/security/auth-and-access-control.md
|
||||
- .agents/review-rules/security/data-and-infrastructure.md
|
||||
- .agents/review-rules/security/node-input-safety.md
|
||||
include:
|
||||
- packages/cli/**
|
||||
- packages/@n8n/db/**
|
||||
- packages/core/**
|
||||
- packages/workflow/**
|
||||
- packages/nodes-base/**
|
||||
- packages/@n8n/nodes-langchain/**
|
||||
exclude:
|
||||
- '**/__tests__/**'
|
||||
- '**/*.test.ts'
|
||||
- '**/*.spec.ts'
|
||||
- '**/test/**'
|
||||
description: |-
|
||||
Proactively review PRs and flag security vulnerabilities specific to n8n's architecture:
|
||||
Flag security defects introduced by this PR, using the linked rules.
|
||||
Each rule file opens with the packages it applies to — skip a file when
|
||||
the changed code is out of its scope.
|
||||
|
||||
**Code Execution & Sandboxing:**
|
||||
- Changes that weaken expression evaluation sandbox protections
|
||||
- Modifications to Code node sandboxes (JavaScript/Python) that reduce isolation
|
||||
- New access to Node.js builtins or external modules in sandboxed contexts
|
||||
- Prototype pollution or constructor access bypasses
|
||||
- Weakening of prototype sanitizers or function context validators
|
||||
- Changes that expose `process.env` access in Code nodes
|
||||
|
||||
**Credential & Secret Handling:**
|
||||
- Credentials being logged or exposed in error messages
|
||||
- Hardcoded secrets, API keys, or tokens in code
|
||||
- Changes to credential encryption/decryption that weaken security
|
||||
- OAuth state/CSRF token handling that bypasses validation
|
||||
- Webhook requests that don't sanitize auth cookies
|
||||
- External secrets provider integrations with insecure configurations
|
||||
- Credential access not respecting scope boundaries (instance/project/user)
|
||||
- Data fetched via credentials being exposed to unauthorized user groups
|
||||
|
||||
**Authentication & Session Security:**
|
||||
- JWT token handling that weakens validation or expiration
|
||||
- Missing or disabled cookie security flags (HttpOnly, Secure, SameSite)
|
||||
- Changes that bypass MFA enforcement
|
||||
- SSO/SAML/OIDC authentication flows with validation gaps
|
||||
- OAuth 2.0 implementations missing PKCE or with weak redirect URI validation
|
||||
|
||||
**Authorization & Access Control:**
|
||||
- Missing or bypassed RBAC and scope-based permission checks
|
||||
- Missing credential permission validation before workflow execution
|
||||
- Subworkflow execution that bypasses caller policies or ownership validation
|
||||
- Missing project-level or resource-level access controls
|
||||
|
||||
**License Enforcement:**
|
||||
- Changes that weaken license enforcement
|
||||
- Missing `FeatureNotLicensedError` for unlicensed feature access
|
||||
- Bypassed license quota checks
|
||||
- New licensed features without proper middleware or module-level enforcement
|
||||
- New controller endpoints in `*.ee.ts` files that access licensed features without `@Licensed` decorator
|
||||
- `@Licensed` decorator usage that doesn't match feature requirements (check `LICENSE_FEATURES` in `@n8n/constants`)
|
||||
- Endpoints in `*.ee.ts` files with `@GlobalScope` or `@ProjectScope` but missing `@Licensed` (license check is separate from permission check)
|
||||
- Preferred pattern: `@Licensed` decorator over custom licensing middleware
|
||||
- Decorator order should be: route decorator → `@Licensed` → scope decorators
|
||||
- Enterprise features not properly isolated in modules (newer features should use the module system)
|
||||
- Enterprise code accessible outside of `*.ee.ts` files or licensed modules
|
||||
- name: Prototype Pollution via Node Parameters
|
||||
Higher scrutiny for the expression engine, credential handling, code
|
||||
execution nodes, license enforcement, and SSO integrations. Community
|
||||
and custom nodes carry a higher risk profile than official ones.
|
||||
- name: Backend
|
||||
file_paths:
|
||||
- .agents/review-rules/backend/controller-request-validation.md
|
||||
- .agents/review-rules/backend/error-classes.md
|
||||
- .agents/review-rules/backend/explicit-any.md
|
||||
- .agents/review-rules/backend/lazy-load-heavy-modules.md
|
||||
- .agents/review-rules/backend/hand-rolled-delays.md
|
||||
include:
|
||||
- packages/cli/**
|
||||
- packages/@n8n/db/**
|
||||
- packages/core/**
|
||||
- packages/workflow/**
|
||||
- packages/nodes-base/**
|
||||
- packages/@n8n/nodes-langchain/**
|
||||
exclude:
|
||||
- '**/__tests__/**'
|
||||
- '**/*.test.ts'
|
||||
- '**/*.spec.ts'
|
||||
- '**/test/**'
|
||||
description: |-
|
||||
- Rule Statement:
|
||||
In node code, a value derived from user-controlled input must never be
|
||||
used as a computed object key in an **assignment** to a plain object.
|
||||
User input reaches nodes mainly via `this.getNodeParameter(...)` (and
|
||||
incoming `item.json` keys), and a workflow author can set such a value
|
||||
to `__proto__`, `constructor`, or `prototype`, polluting the prototype
|
||||
chain (denial of service, and worse depending on the sink).
|
||||
Review n8n's backend and node packages against the linked rules. Think in
|
||||
terms of blast radius, resource cost, and failure modes. Be pragmatic,
|
||||
not pedantic.
|
||||
|
||||
Use `setSafeObjectProperty(target, key, value)` /
|
||||
`isSafeObjectProperty(key)` from `n8n-workflow`, a `Map`, or a
|
||||
null-prototype object (`Object.create(null)`) instead.
|
||||
|
||||
- Detection Criteria:
|
||||
Flag NEW code in `packages/nodes-base/**` or
|
||||
`packages/@n8n/nodes-langchain/**` when a value that traces back to
|
||||
`this.getNodeParameter(...)` (directly, via a variable, via a
|
||||
`.map`/`.reduce`/`.forEach` callback param, or via a `getNodeParam`
|
||||
function passed into a helper such as `createTableStruct`) is used as a
|
||||
computed key to BUILD A NESTED OR CONTAINER STRUCTURE on a plain object
|
||||
(an `IDataObject` / object literal — not a `Map`, not `Object.create(null)`):
|
||||
1. Container creation: `obj[key] = {}` or `obj[key] = []`
|
||||
2. Nested write where the untrusted value is a key: `obj[k1][k2] = value`
|
||||
3. The same via `obj[key] ??= {}` / `obj[key] ||= []`
|
||||
This nested/container shape is what actually pollutes `Object.prototype`
|
||||
(typically the dangerous pattern is building a shared accumulator, e.g.
|
||||
`if (acc[table] === undefined) acc[table] = {}` then `acc[table][key] = []`).
|
||||
|
||||
- Do NOT flag:
|
||||
- Reads: `const x = obj[key]`, `if (obj[key] === undefined)`
|
||||
(reads alone do not pollute)
|
||||
- Single-level writes of a concrete (non-object) value, e.g.
|
||||
`item.json[field] = value`, `item.binary[prop] = data`,
|
||||
`body[target] = value`. Assigning a primitive to `__proto__` is a
|
||||
no-op, and setting one property on a fresh per-item object is not
|
||||
prototype pollution. This is the overwhelmingly common, benign case
|
||||
in nodes — flagging it is noise.
|
||||
- Keys that are string/number literals, or are validated by
|
||||
`isSafeObjectProperty(key)` before the write
|
||||
- Writes routed through `setSafeObjectProperty(...)`
|
||||
- Targets that are a `Map` or created with `Object.create(null)`
|
||||
- Existing unchanged code (review only new or modified lines)
|
||||
|
||||
- Example Violation:
|
||||
```typescript
|
||||
const table = this.getNodeParameter('table', i) as string;
|
||||
const key = this.getNodeParameter('deleteKey', i) as string;
|
||||
if (acc[table] === undefined) acc[table] = {}; // acc['__proto__'] = {}
|
||||
if (acc[table][key] === undefined) acc[table][key] = [];
|
||||
```
|
||||
|
||||
- Example Allowed:
|
||||
```typescript
|
||||
import { setSafeObjectProperty, isSafeObjectProperty } from 'n8n-workflow';
|
||||
|
||||
if (isSafeObjectProperty(table) && acc[table] === undefined) {
|
||||
setSafeObjectProperty(acc, table, {});
|
||||
}
|
||||
// or: const acc = new Map<string, ...>();
|
||||
```
|
||||
|
||||
- Recommendation:
|
||||
Prefer `setSafeObjectProperty` / `isSafeObjectProperty` from
|
||||
`n8n-workflow`. See `nodes/Google/GSuiteAdmin/GSuiteAdmin.node.ts` and
|
||||
`nodes/HttpRequest/V3/HttpRequestV3.node.ts` for reference usage.
|
||||
- name: Use DTOs for Request Body Validation
|
||||
The rules are drawn from real incidents and the conventions the team
|
||||
enforces. Each file opens with the packages it applies to — skip one when
|
||||
the changed code is out of its scope. If none match, say nothing.
|
||||
- name: Frontend
|
||||
file_paths:
|
||||
- .agents/skills/design-system/SKILL.md
|
||||
- .agents/review-rules/frontend/design-system.md
|
||||
- .agents/review-rules/frontend/workflow-document-store.md
|
||||
include:
|
||||
- packages/frontend/**
|
||||
exclude:
|
||||
- '**/__tests__/**'
|
||||
- '**/*.stories.ts'
|
||||
- '**/*.spec.ts'
|
||||
description: |-
|
||||
- Rule Statement:
|
||||
Controller endpoints that accept request bodies must use the `@Body` decorator with a DTO class for runtime validation. TypeScript types alone provide no runtime validation - only `@Body` with a Zod-based DTO (extending `Z.class`) validates incoming data.
|
||||
|
||||
- Detection Criteria:
|
||||
Only flag in `*.controller.ts` files when there is positive evidence the developer intended to accept a body but didn't use the pattern:
|
||||
|
||||
1. Parameter named `payload`, `body`, or `data` without `@Body` decorator
|
||||
2. Direct `req.body` access in a controller method
|
||||
3. `@Body` decorator with a type not ending in `Dto` (indicates unawareness of the DTO pattern)
|
||||
|
||||
Do NOT flag:
|
||||
- `@Body` with a type ending in `Dto` (developer knows the pattern)
|
||||
- POST/PUT/PATCH endpoints without body parameters (may legitimately have no body)
|
||||
- Webhook controllers
|
||||
- Existing unchanged code
|
||||
|
||||
- Example Violation:
|
||||
```typescript
|
||||
@Post('/')
|
||||
async create(req: AuthenticatedRequest, payload: CreateUser) {
|
||||
// payload is undefined - missing @Body decorator
|
||||
}
|
||||
|
||||
@Post('/')
|
||||
async create(req: AuthenticatedRequest) {
|
||||
const data = req.body; // No runtime validation
|
||||
}
|
||||
|
||||
@Post('/')
|
||||
async create(@Body payload: CreateUser) {
|
||||
// Type doesn't end in Dto - likely missing Zod validation
|
||||
}
|
||||
```
|
||||
|
||||
- Example Allowed:
|
||||
```typescript
|
||||
@Post('/')
|
||||
async create(@Body payload: CreateUserDto) { ... }
|
||||
|
||||
@Post('/activate')
|
||||
async activate(req: AuthenticatedRequest) {
|
||||
// Legitimately no body needed
|
||||
}
|
||||
```
|
||||
|
||||
- Recommendation:
|
||||
Use `@Body` decorator with a DTO class from `@n8n/api-types` or a local `dto/` directory. DTOs must extend `Z.class` from `zod-class` for runtime validation.
|
||||
|
||||
**Input Validation & Injection Prevention:**
|
||||
- Unsanitized user input flowing to file paths
|
||||
- SQL nodes with expressions in query fields without parameterization
|
||||
- Command execution nodes with unsanitized shell arguments
|
||||
- Path traversal risks in file operation nodes
|
||||
- Community package names with suspicious patterns
|
||||
|
||||
**File System Access:**
|
||||
- Changes that weaken file access restriction enforcement
|
||||
- File operations that bypass allowlist/blocklist patterns
|
||||
- Access to n8n internal directories
|
||||
|
||||
**Database Security & Performance:**
|
||||
- Missing indexes on frequently queried columns in migrations
|
||||
- Resource-intensive queries without pagination or limits
|
||||
- Missing encryption for sensitive fields beyond credentials
|
||||
- Raw SQL queries that bypass TypeORM protections
|
||||
|
||||
**HTTP, Webhooks & Network Security:**
|
||||
- SSRF risks in user-controlled URLs
|
||||
- CORS configuration that allows all origins
|
||||
- CSP or iframe sandbox modifications that weaken protections
|
||||
- Rate limiting configuration changes that reduce protection
|
||||
- Disabled TLS certificate validation (`rejectUnauthorized: false`)
|
||||
|
||||
**Audit Logging & Event Bus:**
|
||||
- Missing logging for security-relevant events
|
||||
- Sensitive data appearing in logs or error messages
|
||||
- Incomplete audit trails for authentication and authorization events
|
||||
|
||||
**Context-aware review:**
|
||||
- Higher scrutiny for: expression engine, credential handling, code execution nodes, license enforcement, SSO integrations
|
||||
- Consider n8n's security audit categories: credentials, database, nodes, instance, filesystem risks
|
||||
- Community/custom nodes have higher risk profile than official nodes
|
||||
- name: Quality & Performance Review
|
||||
Enforce n8n's frontend conventions using the linked rules. The design
|
||||
system skill is the source of truth for which token to reach for; the
|
||||
rule files that follow set the enforcement level.
|
||||
- name: QA & DX
|
||||
file_paths:
|
||||
- .agents/review-rules/qa-dx/docker-native-modules.md
|
||||
- .agents/review-rules/qa-dx/docker-image-pinning.md
|
||||
- .agents/review-rules/qa-dx/workflow-safety.md
|
||||
- .agents/review-rules/qa-dx/ratchets-and-allowlists.md
|
||||
include:
|
||||
- .github/**
|
||||
- docker/**
|
||||
- scripts/**
|
||||
- patches/**
|
||||
- packages/testing/**
|
||||
- packages/@n8n/eslint-config/**
|
||||
- packages/@n8n/stylelint-config/**
|
||||
- packages/@n8n/vitest-config/**
|
||||
- packages/@n8n/typescript-config/**
|
||||
- '**/eslint.config.mjs'
|
||||
- '**/vitest.config.*'
|
||||
- '**/turbo.json'
|
||||
- .poutine.yml
|
||||
- codecov.yml
|
||||
- .code-health-baseline.json
|
||||
- .boundaries-baseline.json
|
||||
- '**/.janitor-baseline.json'
|
||||
description: |-
|
||||
You are a Staff Quality Engineer reviewing this PR for production
|
||||
readiness. You think in terms of blast radius, resource cost, and
|
||||
failure modes. You are pragmatic, not pedantic.
|
||||
Review the build, test, and CI surface against the linked rules. These
|
||||
are lessons from builds that actually broke and from checks that turned
|
||||
out to be bypassable, not general Docker or Actions advice.
|
||||
|
||||
Below is a curated list of known issues from real incidents and
|
||||
review standards the team enforces. Review the diff against each
|
||||
item. If none match, say nothing. If one matches, explain the
|
||||
concrete risk and suggest a fix. Never flag existing unchanged code.
|
||||
Most of this surface is already scanned — Poutine, Zizmor, and
|
||||
`@n8n/code-health` fail CI for the well-known hazards. Report what those
|
||||
cannot see: a documented invariant being undone, a guard being widened,
|
||||
a gate that stops being able to fail.
|
||||
|
||||
## Known Issues
|
||||
|
||||
### 1. Eager imports of heavy or native modules
|
||||
Top-level `import` of modules only used in a specific code path loads
|
||||
them into every process at startup, increasing baseline memory.
|
||||
Native modules (e.g. `isolated-vm`) can crash instances that lack the
|
||||
binary. Large parsers (e.g. `jsdom` at ~16 MB heap) waste memory when
|
||||
the code path is rarely hit.
|
||||
**Fix:** Use `await import()` at point of use. For barrel files, use
|
||||
`export type` instead of value re-exports when consumers only need
|
||||
the type.
|
||||
|
||||
### 2. Unreasonable test coverage expectations
|
||||
Be pragmatic about test requirements. Pass if core functionality and
|
||||
critical paths are tested at reasonable coverage. Do NOT require tests
|
||||
for exports, types, configs, metadata files, or version files. Let
|
||||
humans handle edge cases.
|
||||
- name: Design System Tokens
|
||||
description: |-
|
||||
Follow `.claude/plugins/n8n/skills/design-system-rules/SKILL.md`.
|
||||
|
||||
Apply balanced enforcement:
|
||||
- Strong warning: hard-coded visual values, legacy token usage, and
|
||||
deprecated style/component surfaces.
|
||||
- Soft warning: token-to-token substitutions (request intent/rationale).
|
||||
- name: Use workflowDocumentStore for Migrated Workflow Fields
|
||||
description: |-
|
||||
We are actively migrating workflow state from `workflowsStore` (Pinia store defined in
|
||||
`packages/frontend/editor-ui/src/app/stores/workflows.store.ts`) to `workflowDocumentStore`
|
||||
(defined in `packages/frontend/editor-ui/src/app/stores/workflowDocument.store.ts`).
|
||||
|
||||
The `workflowDocumentStore` is the single source of truth for migrated fields.
|
||||
When new frontend code reads or writes any of the fields listed below, it **must** use
|
||||
`workflowDocumentStore` instead of `workflowsStore.workflow.*` or equivalent accessors.
|
||||
|
||||
**Already-migrated fields (do NOT access via workflowsStore):**
|
||||
- `active`
|
||||
- `activeVersion`
|
||||
- `activeVersionId`
|
||||
- `checksum`
|
||||
- `createdAt`
|
||||
- `homeProject`
|
||||
- `meta`
|
||||
- `pinData`
|
||||
- `settings`
|
||||
- `tags`
|
||||
- `updatedAt`
|
||||
|
||||
**Detection criteria — flag when NEW code in `packages/frontend/` does any of:**
|
||||
1. Reads a migrated field from `workflowsStore.workflow.<field>` (e.g. `workflowsStore.workflow.tags`)
|
||||
2. Reads a migrated field from a destructured `workflow` ref originating from `workflowsStore`
|
||||
3. Writes to a migrated field through `workflowsStore` (e.g. `workflowsStore.workflow.active = true`)
|
||||
4. Calls a `workflowsStore` setter/action that only mutates a migrated field when
|
||||
`workflowDocumentStore` already exposes an equivalent method
|
||||
|
||||
**Do NOT flag:**
|
||||
- Existing unchanged code (only review new or modified lines)
|
||||
- Access to non-migrated fields (e.g. `workflowsStore.workflow.nodes`, `workflowsStore.workflow.connections`)
|
||||
- Backend code — this rule applies only to `packages/frontend/`
|
||||
- Code inside `workflowDocument.store.ts` or its sub-modules (those *are* the source of truth)
|
||||
- Code inside `workflows.store.ts` itself (the migration is still in progress there)
|
||||
|
||||
**Recommendation:**
|
||||
Use `useWorkflowDocumentStore(createWorkflowDocumentId(workflowId))` from
|
||||
`@/app/stores/workflowDocument.store.ts` to access migrated fields.
|
||||
|
||||
This is an active migration. The list of migrated fields will grow over time.
|
||||
- name: Use the Shared sleep Utility
|
||||
description: |-
|
||||
- Rule Statement:
|
||||
Code must ALWAYS use `sleep` from `@n8n/utils/sleep` instead of
|
||||
hand-rolling a helper that only wraps `setTimeout` in a promise. One
|
||||
canonical helper keeps call sites consistent and makes its abortable
|
||||
form discoverable — `sleep(ms, abortSignal?)` rejects early when the
|
||||
signal fires, so a local abort-aware waiter is a violation too.
|
||||
|
||||
The `no-restricted-sleep-definition` and `no-restricted-sleep-import`
|
||||
ESLint rules already catch helpers literally named `sleep` /
|
||||
`sleepWithAbort` and named `sleep` imports from `n8n-workflow`. Do not
|
||||
repeat those as review comments — this rule covers only what they
|
||||
cannot see syntactically.
|
||||
|
||||
- Detection Criteria:
|
||||
Flag NEW or MODIFIED code in `packages/**` where a delay is
|
||||
re-implemented under a name ESLint does not restrict (`wait`, `delay`,
|
||||
`pause`, `waitFor`, …) and the entire body is a promise wrapping
|
||||
`setTimeout` with no additional behaviour:
|
||||
- `const delay = (ms) => new Promise((resolve) => setTimeout(resolve, ms))`
|
||||
- `async function wait(ms) { await new Promise((r) => setTimeout(r, ms)); }`
|
||||
The shape is what matters, not the name. Also flag an inline, awaited
|
||||
`new Promise((resolve) => setTimeout(resolve, ms))`.
|
||||
|
||||
- Do NOT flag:
|
||||
- Wrappers that do more than wait: racing against another promise,
|
||||
rejecting with a timeout error once the delay elapses, retry
|
||||
backoff with jitter, debounce/throttle. (Rejecting on *abort* is a
|
||||
violation — `sleep` takes the signal.)
|
||||
- Bare `setTimeout` calls used for scheduling, not awaited as a delay
|
||||
- Test-framework helpers such as `vi.advanceTimersByTime` or waiting
|
||||
on fake timers
|
||||
- `packages/@n8n/node-cli/**` and `packages/@n8n/typeorm/**`, which
|
||||
cannot depend on `@n8n/utils` and are exempt from the ESLint rules
|
||||
- Existing unchanged code (review only new or modified lines)
|
||||
|
||||
- Recommendation:
|
||||
Import `sleep` from `@n8n/utils/sleep` — `await sleep(500)`, or
|
||||
`await sleep(500, abortSignal)` when the wait must be cancellable.
|
||||
# cubic silently drops any rule past the fifth, so the last slot is a decision,
|
||||
# not somewhere to append. Merge into an existing agent instead.
|
||||
pr_descriptions:
|
||||
generate: false
|
||||
instructions: Each PR is supposed to have a limited scope. In your review, focus on changes made in the PR and avoid pointing out problems you found in the code that already existed.
|
||||
issues:
|
||||
fix_with_cubic_buttons: true
|
||||
|
|
|
|||
|
|
@ -52,6 +52,7 @@
|
|||
"sync:skill-links": "node scripts/sync-agent-skill-links.mjs",
|
||||
"check:skill-links": "node scripts/sync-agent-skill-links.mjs --check",
|
||||
"check:workspace-private-deps": "node scripts/check-workspace-private-deps.mjs",
|
||||
"check:cubic-config": "node .github/scripts/quality/check-cubic-config.mjs",
|
||||
"optimize-svg": "find ./packages -name '*.svg' ! -name 'pipedrive.svg' -print0 | xargs -0 -P16 -L20 npx svgo",
|
||||
"n8n-module-sdk": "node packages/@n8n/module-cli/bin/n8n-module-sdk.mjs",
|
||||
"setup-backend-module": "node scripts/ensure-zx.mjs && zx scripts/backend-module/setup.mjs",
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user