ci: Improve cubic prompts and enable shadowing (#37275)

.agents/review-rules/README.md

@@ -5,6 +5,10 @@ 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.
 
+These files hold *what to look for*. How to review — the bar a comment must
+clear, when to stay silent, what never to say — lives once in
+`custom_instructions` in `cubic.yaml`, because it reaches every agent.
+
 ## Layout
 
 | Directory   | Agent    | Scope                                               |
@@ -13,6 +17,13 @@ as normal markdown.
 | `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 |
+| `testing/`  | Backend + Frontend | any package with a test suite            |
+
+A directory maps to one agent unless, like `testing/`, the policy is identical
+across domains — then it is one file listed in several agents' `file_paths`,
+never a copy per directory. Security and QA & DX deliberately don't link
+`testing/`: coverage nagging on a credential fix or a Dockerfile is noise those
+agents shouldn't be able to produce.
 
 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
@@ -26,20 +37,26 @@ 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.
+- **10,000 characters per agent**, counting the `description`, every linked
+  file, and `custom_instructions` — concatenated in the listed order.
+  Everything past the limit is dropped from the review prompt. The shared block
+  is prepended to every agent, so a line added there is spent against all of
+  their ceilings, not one; `check:cubic-config` prints the own/shared split.
 - **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
+1. Pick the level of reach first. Guidance about *how* to review goes in
+   `custom_instructions`, not here. A policy that is word-for-word the same in
+   two domains becomes one file linked by both agents. Everything else goes in
+   the directory of the agent that owns it.
+2. 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.
+3. Add its path to every `file_paths` that should load it in `cubic.yaml`.
+4. 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.
 
@@ -49,8 +66,9 @@ 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.
+Don't restate something CI already fails on — see the "Don't repeat what CI
+already fails on" section in `cubic.yaml`. Write what static analysis cannot
+see.
 
 ## Working with cubic on a PR
 

.agents/review-rules/backend/hand-rolled-delays.md

@@ -18,5 +18,7 @@ Do NOT flag:
   timeout error, retry backoff, debounce/throttle
 - Bare `setTimeout` used for scheduling rather than awaited as a delay
 - Fake timer helpers such as `vi.advanceTimersByTime`
+- Helpers literally named `sleep`/`sleepWithAbort`, and `sleep` imported from
+  `n8n-workflow` — ESLint already fails the build for both
 - `packages/@n8n/node-cli/**` and `packages/@n8n/typeorm/**`, which cannot depend
   on `@n8n/utils`

.agents/review-rules/frontend/design-system.md

@@ -6,7 +6,8 @@ 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.
+nothing catches a hard-coded one. Never comment on the naming itself — that
+half already fails the build.
 
 - Strong warning: hard-coded visual values (px, rem, hex colours, durations)
   where a token exists; legacy token usage; deprecated style or component

.agents/review-rules/testing/coverage.md

@@ -0,0 +1,14 @@
+# Test coverage
+
+Applies to: any package with a test suite.
+
+Flag new behaviour that ships with no test — a service method, controller
+route, node operation, store action, or composable. Coverage does not need to
+be complete; core functionality and the critical path do.
+
+Do NOT flag:
+
+- Exports, types, configuration objects, metadata, version files
+- A percentage — the number is not the bar, the critical path is
+- Edge cases a human reviewer is better placed to ask for
+- A skipped test — the `no-skipped-tests` ESLint rule already fails the build

.github/scripts/quality/check-cubic-config.mjs

@@ -26,7 +26,11 @@ 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. */
+/**
+ * Description, linked file contents, *and* `reviews.custom_instructions`;
+ * characters past this are dropped from the prompt. The shared block is
+ * prepended to every agent, so it is spent once per agent, not once overall.
+ */
 export const MAX_RULE_CHARS = 10_000;
 
 /** Fraction of the ceiling at which a rule is reported as close to silent truncation. */
@@ -105,7 +109,7 @@ function ruleFiles() {
  * @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> }}
+ * @returns {{ violations: string[], warnings: string[], ruleLengths: Record<string, number>, sharedChars: number }}
  */
 export function checkConfig(config, charsIn, onDisk = []) {
 	const violations = [];
@@ -119,10 +123,23 @@ export function checkConfig(config, charsIn, onDisk = []) {
 		violations.push(`\`version\` must be 1, found ${JSON.stringify(config?.version)}.`);
 	}
 
+	// Prepended to every agent's prompt, so it is charged against each one's
+	// ceiling separately. A line added here is paid as many times as there are
+	// agents.
+	const sharedChars = (config?.reviews?.custom_instructions ?? '').length;
+
+	if (sharedChars >= MAX_RULE_CHARS) {
+		violations.push(
+			`\`reviews.custom_instructions\` is ${sharedChars.toLocaleString()} characters, which ` +
+				`alone fills the ${MAX_RULE_CHARS.toLocaleString()}-character ceiling every agent ` +
+				`shares. No agent's own rules would survive.`,
+		);
+	}
+
 	const rules = config?.reviews?.custom_rules ?? [];
 	if (!Array.isArray(rules)) {
 		violations.push('`reviews.custom_rules` must be a list.');
-		return { violations, warnings, ruleLengths };
+		return { violations, warnings, ruleLengths, sharedChars };
 	}
 
 	if (rules.length > MAX_CUBIC_AGENTS) {
@@ -147,7 +164,7 @@ export function checkConfig(config, charsIn, onDisk = []) {
 			violations.push(`${label} needs a \`description\`, \`file_paths\`, or both.`);
 		}
 
-		let total = description.length;
+		let total = sharedChars + description.length;
 		for (const path of filePaths) {
 			linked.add(path);
 			const chars = charsIn(path);
@@ -158,15 +175,23 @@ export function checkConfig(config, charsIn, onDisk = []) {
 			total += chars;
 		}
 
+		// Spell out the split: an agent can bust the ceiling on shared text alone,
+		// and trimming its own rules would not be the fix.
+		const split = sharedChars
+			? ` (${(total - sharedChars).toLocaleString()} its own + ` +
+				`${sharedChars.toLocaleString()} shared \`custom_instructions\`)`
+			: '';
+
 		if (total > MAX_RULE_CHARS) {
 			violations.push(
-				`${label} is ${total.toLocaleString()} characters; everything past ` +
+				`${label} is ${total.toLocaleString()} characters${split}; 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.`,
+					`${MAX_RULE_CHARS.toLocaleString()}-character ceiling${split}. ` +
+					'Trim it before adding more.',
 			);
 		}
 
@@ -179,7 +204,7 @@ export function checkConfig(config, charsIn, onDisk = []) {
 		}
 	}
 
-	return { violations, warnings, ruleLengths };
+	return { violations, warnings, ruleLengths, sharedChars };
 }
 
 async function refreshSchema() {
@@ -202,9 +227,17 @@ async function main() {
 	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());
+	const { violations, warnings, ruleLengths, sharedChars } = checkConfig(
+		config,
+		fileCharacters,
+		ruleFiles(),
+	);
 	violations.unshift(...schemaErrors(config, schema));
 
+	console.log(
+		`Shared \`custom_instructions\`: ${sharedChars.toLocaleString()} characters, ` +
+			'included in every agent below.',
+	);
 	console.log("Rule sizes:")
 	for (const [label, ruleLength] of Object.entries(ruleLengths)) {
 		console.log(`	${label}: ${ruleLength} characters (${Math.floor(ruleLength / MAX_RULE_CHARS * 100)}%)`);

.github/scripts/quality/check-cubic-config.test.mjs

@@ -20,8 +20,11 @@ before(async () => {
 /** 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 {Array<object>} rules @param {string} [customInstructions] */
+const config = (rules, customInstructions) => ({
+	version: 1,
+	reviews: { custom_rules: rules, ...(customInstructions && { custom_instructions: customInstructions }) },
+});
 
 /** @param {string} name */
 const rule = (name) => ({ name, description: 'x' });
@@ -132,6 +135,72 @@ describe('checkConfig', () => {
 	});
 });
 
+describe('shared custom_instructions', () => {
+	it('charges the shared block against every agent, not once overall', () => {
+		const rules = [{ name: 'A', description: 'a'.repeat(100) }, { name: 'B', description: 'b'.repeat(200) }];
+		const { ruleLengths, sharedChars } = checkConfig(config(rules, 'i'.repeat(1_000)), emptyFiles);
+
+		assert.equal(sharedChars, 1_000);
+		assert.equal(ruleLengths['"A"'], 1_100);
+		assert.equal(ruleLengths['"B"'], 1_200);
+	});
+
+	it('fails an agent that only busts the ceiling once the shared block is added', () => {
+		const rules = [{ name: 'Security', description: 'a'.repeat(9_500) }];
+
+		assert.deepEqual(violationsOf(config(rules), emptyFiles), []);
+
+		const violations = violationsOf(config(rules, 'i'.repeat(1_000)), emptyFiles);
+		assert.equal(violations.length, 1);
+		assert.match(violations[0], /"Security" is 10,500 characters/);
+	});
+
+	it('splits own and shared characters so the fix is obvious', () => {
+		const rules = [{ name: 'Security', description: 'a'.repeat(9_500) }];
+		const [violation] = violationsOf(config(rules, 'i'.repeat(1_000)), emptyFiles);
+
+		assert.match(violation, /9,500 its own \+ 1,000 shared `custom_instructions`/);
+	});
+
+	it('splits own and shared characters in the warning too', () => {
+		const rules = [{ name: 'Security', description: 'a'.repeat(7_500) }];
+		const { violations, warnings } = checkConfig(config(rules, 'i'.repeat(1_000)), emptyFiles);
+
+		assert.deepEqual(violations, []);
+		assert.equal(warnings.length, 1);
+		assert.match(warnings[0], /at 85% of the 10,000-character ceiling/);
+		assert.match(warnings[0], /7,500 its own \+ 1,000 shared `custom_instructions`/);
+	});
+
+	it('omits the split when there is no shared block', () => {
+		const rules = [{ name: 'Security', description: 'a'.repeat(10_001) }];
+		const [violation] = violationsOf(config(rules), emptyFiles);
+
+		assert.doesNotMatch(violation, /its own/);
+	});
+
+	it('flags a shared block that fills the ceiling on its own', () => {
+		const rules = [{ name: 'A', description: 'x' }];
+		const violations = violationsOf(config(rules, 'i'.repeat(MAX_RULE_CHARS)), emptyFiles);
+
+		assert.match(violations[0], /`reviews\.custom_instructions` is 10,000 characters/);
+		assert.match(violations[0], /No agent's own rules would survive/);
+	});
+
+	it('reports sharedChars as 0 when the block is absent', () => {
+		assert.equal(checkConfig(config([rule('A')]), emptyFiles).sharedChars, 0);
+	});
+
+	it('measures the shared block in characters, not UTF-8 bytes', () => {
+		const rules = [{ name: 'A', description: '' , file_paths: ['doc.md'] }];
+		// 4,000 em dashes: 12,000 bytes, 4,000 characters. By bytes this would fail.
+		const { violations, sharedChars } = checkConfig(config(rules, '—'.repeat(4_000)), () => 5_000);
+
+		assert.equal(sharedChars, 4_000);
+		assert.deepEqual(violations, []);
+	});
+});
+
 describe('ceiling warnings', () => {
 	it('warns without failing once a rule passes 80%', () => {
 		const rules = [{ name: 'Security', description: 'a'.repeat(8_500) }];

cubic.yaml

@@ -6,8 +6,15 @@
 # Settings defined here take precedence over UI-configured settings.
 # See https://docs.cubic.dev/configure/cubic-yaml for documentation.
 #
+# Guidance lives at one of three levels of reach: `custom_instructions` for what
+# is true of every agent, a rule file linked by several agents for a policy that
+# is identical across domains, and an agent's own directory for everything else.
+# `.agents/review-rules/README.md` explains the split.
+#
 # 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.
+# characters — its description, its linked file_paths, and custom_instructions,
+# which is prepended to every agent and so is charged against each one separately.
+# `pnpm check:cubic-config` enforces both limits and prints the split.
 # Scope every rule with include/exclude so it is not evaluated against the whole monorepo.
 
 version: 1
@@ -16,6 +23,18 @@ reviews:
   sensitivity: medium
   incremental_commits: true
   check_drafts: true
+  architecture_diagrams: false
+  merge_confidence_summary: true
+
+  # Both of these turn on a risk judgement this config deliberately does not
+  # define — guessing the scale up front would just bake in a wrong one. Shadow
+  # mode makes a miscalibrated call free, so calibrate from what cubic actually
+  # classifies before writing a risk section into custom_instructions.
+  auto_approve_behavior: shadow
+  auto_approve: low_risk_only
+
+  auto_ultrareview: high_risk_only
+
   ignore:
     # Mechanical PRs: content was already reviewed on master, or there is nothing
     # to review. Backports stay reviewed — their conflicts are resolved by hand.
@@ -36,47 +55,51 @@ reviews:
       - '**/*.snap'
       - '**/*.generated.yml'
       - '**/*.generated.ts'
+  # Reaches every agent, so it holds only how to review — never what to look
+  # for. Domain rules belong in a rule file under `.agents/review-rules/`.
   custom_instructions: |-
+    ## The bar for a comment
+
+    Priority order: architecture fit, solution complexity, bugs and behavioural
+    regressions, security edge cases, code quality, missing tests. Style and
+    naming last, and only when they genuinely matter.
+
+    A comment must name the concrete failure — the input, state, or sequence that
+    makes the code wrong — and what to do instead: the snippet, or the specific
+    function to reach for. "This could be cleaner" is noise. Zero comments on a
+    clean PR is a good review; do not manufacture findings.
+
+    Where a deliberate decision and a mistake look identical, ask what the intent
+    was rather than asserting a defect.
+
     ## Scope
 
     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.
 
-    ## Don't repeat the linter
+    Each linked rule file opens with the packages it applies to. Skip a file when
+    the changed code is out of its scope; if none apply, say nothing.
 
-    `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.
+    ## Don't repeat what CI already fails on
 
-    ## Test coverage
+    `pnpm lint`, `pnpm typecheck`, Poutine, Zizmor, and `@n8n/code-health` run on
+    every PR. A defect one of them already fails the build for is not a finding —
+    the author sees it before you do. The linked rule files name the specific
+    exemptions. Report what static analysis cannot see.
 
-    BE REASONABLE when evaluating test coverage.
+    ## This is a public repository
 
-    **PASS if:**
-
-    - Core functionality has tests
-    - Critical paths are tested
-    - Coverage is reasonable (not necessarily 100%)
-
-    **DO NOT require tests for:**
-
-    - Exports, types, configs
-    - Metadata files
-    - Version files
-
-    Approve if reasonably tested. Let humans handle edge cases.
+    Your comments are public. On a change that looks security-related, describe the
+    defect in functional terms — do not spell out an exploit path, name a
+    vulnerability class, or speculate about attacks in the thread. Never name a
+    customer; say "a customer" or "a large deployment" instead.
 
     ## Community contributions
 
-    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.
+    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
       file_paths:
@@ -99,8 +122,6 @@ reviews:
         - '**/test/**'
       description: |-
         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.
 
         Higher scrutiny for the expression engine, credential handling, code
         execution nodes, license enforcement, and SSO integrations. Community
@@ -112,6 +133,7 @@ reviews:
         - .agents/review-rules/backend/explicit-any.md
         - .agents/review-rules/backend/lazy-load-heavy-modules.md
         - .agents/review-rules/backend/hand-rolled-delays.md
+        - .agents/review-rules/testing/coverage.md
       include:
         - packages/cli/**
         - packages/@n8n/db/**
@@ -126,17 +148,20 @@ reviews:
         - '**/test/**'
       description: |-
         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.
+        terms of blast radius, resource cost, and failure modes.
 
         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.
+        enforces.
+
+        ESLint already fails the build for `@n8n/typeorm` imports in
+        `packages/cli` business logic, uncaught `JSON.parse`, and
+        `JSON.parse(JSON.stringify())` — never spend a comment on those.
     - 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
+        - .agents/review-rules/testing/coverage.md
       include:
         - packages/frontend/**
       exclude:
@@ -176,10 +201,8 @@ reviews:
         are lessons from builds that actually broke and from checks that turned
         out to be bypassable, not general Docker or Actions advice.
 
-        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.
+        What this surface's scanners cannot see: a documented invariant being
+        undone, a guard being widened, a gate that stops being able to fail.
 
   # 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.

package.json

@@ -52,7 +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",
+    "check:cubic-config": "pnpm install-workflow-scripts && 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",
@@ -77,7 +77,8 @@
     "webhook": "./packages/cli/bin/n8n webhook",
     "worker": "./packages/cli/bin/n8n worker",
     "dev:computer-use": "pnpm --filter @n8n/computer-use build && node packages/@n8n/computer-use/dist/cli.js serve",
-    "stop:computer-use": "lsof -ti :7655 | xargs kill 2>/dev/null; echo 'computer-use stopped'"
+    "stop:computer-use": "lsof -ti :7655 | xargs kill 2>/dev/null; echo 'computer-use stopped'",
+    "install-workflow-scripts": "pnpm install --dir .github/scripts --ignore-workspace"
   },
   "devDependencies": {
     "@babel/preset-env": "^7.26.0",