From d904f04b5bfc0247961fd3a2a548f39dcedb7ce6 Mon Sep 17 00:00:00 2001 From: Matsu Date: Fri, 28 Aug 2026 07:15:09 +0000 Subject: [PATCH] ci: Improve cubic prompts and enable shadowing (#37275) --- .agents/review-rules/README.md | 38 +++++-- .../backend/hand-rolled-delays.md | 2 + .../review-rules/frontend/design-system.md | 3 +- .agents/review-rules/testing/coverage.md | 14 +++ .../scripts/quality/check-cubic-config.mjs | 49 +++++++-- .../quality/check-cubic-config.test.mjs | 73 +++++++++++++- cubic.yaml | 99 ++++++++++++------- package.json | 5 +- 8 files changed, 222 insertions(+), 61 deletions(-) create mode 100644 .agents/review-rules/testing/coverage.md diff --git a/.agents/review-rules/README.md b/.agents/review-rules/README.md index 2f4ce9dc976..ec65c84ad37 100644 --- a/.agents/review-rules/README.md +++ b/.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 diff --git a/.agents/review-rules/backend/hand-rolled-delays.md b/.agents/review-rules/backend/hand-rolled-delays.md index ee244a28cff..97619119369 100644 --- a/.agents/review-rules/backend/hand-rolled-delays.md +++ b/.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` diff --git a/.agents/review-rules/frontend/design-system.md b/.agents/review-rules/frontend/design-system.md index 480b727f5ed..469931dc89d 100644 --- a/.agents/review-rules/frontend/design-system.md +++ b/.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 diff --git a/.agents/review-rules/testing/coverage.md b/.agents/review-rules/testing/coverage.md new file mode 100644 index 00000000000..21715dfecf9 --- /dev/null +++ b/.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 diff --git a/.github/scripts/quality/check-cubic-config.mjs b/.github/scripts/quality/check-cubic-config.mjs index 21a81596dbb..2f82cab9955 100644 --- a/.github/scripts/quality/check-cubic-config.mjs +++ b/.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 }} + * @returns {{ violations: string[], warnings: string[], ruleLengths: Record, 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)}%)`); diff --git a/.github/scripts/quality/check-cubic-config.test.mjs b/.github/scripts/quality/check-cubic-config.test.mjs index 1ff18ab8ba8..52a4b2b7e23 100644 --- a/.github/scripts/quality/check-cubic-config.test.mjs +++ b/.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} rules */ -const config = (rules) => ({ version: 1, reviews: { custom_rules: rules } }); +/** @param {Array} 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) }]; diff --git a/cubic.yaml b/cubic.yaml index afee39d835a..fb02974860c 100644 --- a/cubic.yaml +++ b/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. diff --git a/package.json b/package.json index cc0e763cf5f..e8b66672214 100644 --- a/package.json +++ b/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",