Skip to content

fix: resolve n8n review findings for 0.2.3 (scanner lint + codex node identifier) #14

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
eitanp461 merged 1 commit into from
Aug 23, 2026
Merged

fix: resolve n8n review findings for 0.2.3 (scanner lint + codex node identifier) #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions credentials/CloudinaryApi.credentials.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@ import {
IAuthenticateGeneric,
ICredentialTestRequest,
ICredentialType,
Icon,
INodeProperties,
} from 'n8n-workflow';

export class CloudinaryApi implements ICredentialType {
name = 'cloudinaryApi';
displayName = 'Cloudinary API';
icon: Icon = { light: 'file:cloudinary.svg', dark: 'file:cloudinary.dark.svg' };

@eitanp461 eitanp461 Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scanner requires credential classes to declare an icon. Note the duplicated SVG files this needs: n8n resolves file: icon paths relative to the declaring class file, so the node resolves into dist/nodes/Cloudinary/ and the credential into dist/credentials/. Per-directory copies are the community-node convention; there's no shared-asset path. All four files are in the packed tarball.

documentationUrl = 'https://cloudinary.com/documentation/developer_onboarding_faq_find_credentials';
properties: INodeProperties[] = [
{
Expand Down
14 changes: 14 additions & 0 deletions credentials/cloudinary.dark.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
14 changes: 14 additions & 0 deletions credentials/cloudinary.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 1 addition & 1 deletion nodes/Cloudinary/Cloudinary.node.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"node": "n8n-nodes-base.cloudinary",
"node": "n8n-nodes-cloudinary.cloudinary",

@eitanp461 eitanp461 Aug 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MEDIUM review finding. Correcting an earlier version of this comment — I claimed this fix also repairs the duplicate-entry behaviour in the Add-action panel. That was wrong, and the history of this file deserves a straight answer, since it has now flipped n8n-nodes-base → n8n-nodes-cloudinary → back → and now forward again (thanks @sveta-slepner for spotting it).

What this field actually does: nothing at runtime. n8n's codex loader reads only four keys out of .node.json. From packages/core/src/nodes-loader/directory-loader.ts:

private getCodex(filePath: string): CodexData {
  const codexFilePath = this.resolvePath(`${filePath}on`); // .js to .json
  const { categories, subcategories, resources: { primaryDocumentation, credentialDocumentation }, alias } = module.require(codexFilePath) as Codex;
  ...
}

node, nodeVersion and codexVersion are never destructured — nothing consumes them. So this value cannot break a workflow, a docs link, or node loading, in either direction.

And the dedupe claim was wrong. The vetted-catalog entry that drives the "install this" suggestion comes from n8n's own Strapi API (api.n8n.io/api/community-nodes), not from our repo: isInstalled(nodeType.name) in community-node-types.service.ts:220 uses the catalog's name, which n8n populates on their side. Our codex file has no input to it. I traced this the wrong way round the first time.

So why change it? One reason only: n8n's 2026-08 review flagged the legacy value as a MEDIUM finding, and that review gates the 0.2.3 publish. Secondarily it's what the codex reference describes — <npm package name>.<node name>, i.e. our package.json name plus the descriptor's name: 'cloudinary' (Cloudinary.node.ts:18), with n8n-nodes-base. reserved for built-ins.

Why the earlier revert looked right. In PR #7 the justification was "the codex spec and published community-node practice". Practice genuinely is inconsistent — I checked published packages: n8n-nodes-brightdata uses n8n-nodes-brightdata.BrightData (our new shape), while n8n-nodes-elevenlabs and n8n-nodes-browserless still ship n8n-nodes-base.*, and a few (n8n-nodes-firecrawl, n8n-nodes-apify) never replaced the template's literal n8n-nodes-<name> placeholder. Nothing enforced it, so both values survived review historically. What's new is that n8n now flags it by hand.

Happy to revert if you'd rather not churn the file again — but then we'd need n8n to waive the finding.

"nodeVersion": "1.0",
"codexVersion": "1.0",
"categories": ["Data & Storage", "Marketing & Content"],
Expand Down
16 changes: 12 additions & 4 deletions nodes/Cloudinary/Cloudinary.node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ import {
INodeTypeDescription,
IExecuteFunctions,
INodeExecutionData,
JsonObject,
NodeApiError,
NodeConnectionTypes,
NodeOperationError,
} from 'n8n-workflow';
import { cloudinaryProperties } from './descriptions';
Expand All @@ -13,7 +16,7 @@ export class Cloudinary implements INodeType {
description: INodeTypeDescription = {
displayName: 'Cloudinary',
name: 'cloudinary',
icon: 'file:cloudinary.svg',
icon: { light: 'file:cloudinary.svg', dark: 'file:cloudinary.dark.svg' },
group: ['transform'],
// v1 exposed the Video Player as flat `player*` params; v2 regrouped them into
// collections (see widget.fields.ts). Both schemas ship side by side, gated by
Expand All @@ -32,8 +35,8 @@ export class Cloudinary implements INodeType {
defaults: {
name: 'Cloudinary',
},
inputs: ['main'],
outputs: ['main'],
inputs: [NodeConnectionTypes.Main],

@eitanp461 eitanp461 Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required by the scanner's @n8n/community-nodes/node-connection-type-literal rule (in its recommended config), which forbids the 'main' string literal and prescribes exactly this form.

Worth naming the trade-off: NodeConnectionTypes only exists in n8n-workflow >= ~1.79, so this raises the package's implicit minimum n8n version, and our peerDependencies is "n8n-workflow": "*". I checked whether keeping the string was an option — it isn't, the gate rejects it. The practical risk is low (the vetted community-node program targets far newer n8n than 1.79), but if we want to be explicit we could declare a floor in a follow-up.

outputs: [NodeConnectionTypes.Main],
credentials: [
{
name: CREDENTIAL_TYPE,
Expand Down Expand Up @@ -84,7 +87,12 @@ export class Cloudinary implements INodeType {
});
continue;
}
throw error;
// Both constructors pass an already-wrapped error of their own type
// through untouched, so handler-thrown errors keep their context.
if (error instanceof NodeApiError) {
throw new NodeApiError(this.getNode(), error as unknown as JsonObject, { itemIndex: i });
}
throw new NodeOperationError(this.getNode(), error as Error, { itemIndex: i });

@eitanp461 eitanp461 Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why construct a new error instead of rethrowing the caught one. require-node-api-error flags any throw <catchParam> inside a catch — even when it's instanceof-guarded — so the previous throw error couldn't stay.

Re-wrapping is safe because it's a pass-through: both constructors return an already-wrapped error of their own class untouched (if (errorResponse instanceof NodeApiError) return errorResponse at node-api.error.js:90, same shape at node-operation.error.js:21 in n8n-workflow 2.16.0). So an error a handler already attributed keeps its message, HTTP code and context.

One consequence: in that pass-through branch the { itemIndex: i } option is discarded, since the constructor returns before applying options. Fine for handler-thrown errors (they set their own item context) — see the note on cloudinary.utils.ts for the one place where it means something.

}
}

Expand Down
14 changes: 14 additions & 0 deletions nodes/Cloudinary/cloudinary.dark.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
20 changes: 13 additions & 7 deletions nodes/Cloudinary/cloudinary.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
42 changes: 26 additions & 16 deletions nodes/Cloudinary/cloudinary.utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { describe, it, expect } from 'vitest';
import type { IDataObject } from 'n8n-workflow';
import type { IDataObject, INode } from 'n8n-workflow';
import {
buildSearchExpression,
createMultipartBody,
Expand Down Expand Up @@ -258,70 +258,80 @@ describe('generateCloudinarySignature', () => {
});

describe('metadataToPipeString', () => {
const node = {
id: '1',
name: 'Cloudinary',
type: 'n8n-nodes-cloudinary.cloudinary',
typeVersion: 2,
position: [0, 0],
parameters: {},
} as INode;
const toPipeString = (input: IDataObject | string) => metadataToPipeString(input, node);

it('joins scalar key/value pairs with pipes', () => {
expect(metadataToPipeString({ a: '1', b: '2' })).toBe('a=1|b=2');
expect(toPipeString({ a: '1', b: '2' })).toBe('a=1|b=2');
});

it('renders array values as a bracketed list of quoted strings', () => {
expect(metadataToPipeString({ colors: ['red', 'blue'] })).toBe('colors=["red","blue"]');
expect(toPipeString({ colors: ['red', 'blue'] })).toBe('colors=["red","blue"]');
});

it('quotes numeric array elements as strings', () => {
expect(metadataToPipeString({ sizes: [1, 2] } as unknown as IDataObject)).toBe(
expect(toPipeString({ sizes: [1, 2] } as unknown as IDataObject)).toBe(
'sizes=["1","2"]',
);
});

it('renders an empty array as empty brackets', () => {
expect(metadataToPipeString({ tags: [] })).toBe('tags=[]');
expect(toPipeString({ tags: [] })).toBe('tags=[]');
});

it('parses a JSON string input', () => {
expect(metadataToPipeString('{"a":"1","b":"2"}')).toBe('a=1|b=2');
expect(toPipeString('{"a":"1","b":"2"}')).toBe('a=1|b=2');
});

it('returns an empty string for an empty object', () => {
expect(metadataToPipeString({})).toBe('');
expect(toPipeString({})).toBe('');
});

it('throws on invalid JSON string input', () => {
expect(() => metadataToPipeString('{not valid}')).toThrow('Invalid JSON for structured metadata');
expect(() => toPipeString('{not valid}')).toThrow('Invalid JSON for structured metadata');
});

it('escapes the pipe delimiter in a scalar value', () => {
expect(metadataToPipeString({ note: 'a|b' })).toBe('note=a\\|b');
expect(toPipeString({ note: 'a|b' })).toBe('note=a\\|b');
});

it('escapes the equals delimiter in a scalar value', () => {
expect(metadataToPipeString({ eq: 'x=y' })).toBe('eq=x\\=y');
expect(toPipeString({ eq: 'x=y' })).toBe('eq=x\\=y');
});

it('escapes both delimiters and keeps following pairs separable', () => {
expect(metadataToPipeString({ a: 'one=two|three', b: 'ok' })).toBe('a=one\\=two\\|three|b=ok');
expect(toPipeString({ a: 'one=two|three', b: 'ok' })).toBe('a=one\\=two\\|three|b=ok');
});

it('escapes double quotes in a scalar value', () => {
expect(metadataToPipeString({ q: 'say "hi"' })).toBe('q=say \\"hi\\"');
expect(toPipeString({ q: 'say "hi"' })).toBe('q=say \\"hi\\"');
});

it('escapes the delimiters (= | ") inside array elements and quote-wraps them', () => {
expect(metadataToPipeString({ tags: ['a|b', 'c=d', 'e"f'] })).toBe(
expect(toPipeString({ tags: ['a|b', 'c=d', 'e"f'] })).toBe(
'tags=["a\\|b","c\\=d","e\\"f"]',
);
});

it('keeps following pairs separable when an array element contains a pipe', () => {
expect(metadataToPipeString({ a: ['x|y'], b: 'ok' })).toBe('a=["x\\|y"]|b=ok');
expect(toPipeString({ a: ['x|y'], b: 'ok' })).toBe('a=["x\\|y"]|b=ok');
});

it('skips null and undefined values rather than emitting key=null', () => {
expect(metadataToPipeString({ a: '1', b: null, c: undefined, d: '2' } as IDataObject)).toBe(
expect(toPipeString({ a: '1', b: null, c: undefined, d: '2' } as IDataObject)).toBe(
'a=1|d=2',
);
});

it('stringifies non-string scalars (numbers, booleans)', () => {
expect(metadataToPipeString({ n: 5, flag: true } as unknown as IDataObject)).toBe(
expect(toPipeString({ n: 5, flag: true } as unknown as IDataObject)).toBe(
'n=5|flag=true',
);
});
Expand Down
9 changes: 5 additions & 4 deletions nodes/Cloudinary/cloudinary.utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { IDataObject, ApplicationError } from 'n8n-workflow';
import { IDataObject, INode, NodeOperationError } from 'n8n-workflow';
import { sha256 } from './sha256.utils';
import { CloudinaryCredentials } from './operations/types';
import { version } from '../../package.json';
Expand Down Expand Up @@ -190,14 +190,15 @@ const escapeMetadataValue = (value: string): string => value.replace(/([=|"])/g,
* matching Cloudinary's multi-value field format. Delimiter characters (`=`, `"`,
* `|`) are backslash-escaped inside every value — scalar and list element alike —
* so a value containing them can't be misparsed as another field, pair, or list
* boundary. Throws ApplicationError on invalid JSON input.
* boundary. Throws NodeOperationError on invalid JSON input, attributed to the
* calling node.
*/
export const metadataToPipeString = (input: IDataObject | string): string => {
export const metadataToPipeString = (input: IDataObject | string, node: INode): string => {

@eitanp461 eitanp461 Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ApplicationError is scanner-flagged, so this now throws NodeOperationError, which needs a node instance — hence the new second parameter. That's the whole reason the four call sites gained ctx.getNode(). User-visible effect: invalid metadata JSON now surfaces as a proper node-attributed error instead of an unattributed internal one.

Known gap, flagged in review: the error carries no itemIndex, and execute() can't add one later (the constructor's pass-through returns before options are applied). So on a multi-item run with bad JSON on item 3, the UI won't point at item 3. Not a regression — the old ApplicationError had neither node nor item attribution — but the fix is small: thread i in from the call sites, which already have it. Happy to do it here or as a follow-up, reviewer's preference.

let metadata: IDataObject;
try {
metadata = typeof input === 'object' ? input : (JSON.parse(input) as IDataObject);
} catch (error) {
throw new ApplicationError('Invalid JSON for structured metadata');
throw new NodeOperationError(node, 'Invalid JSON for structured metadata');
}
return Object.keys(metadata)
.map((key) => {
Expand Down
Loading
Loading