diff --git a/packages/@n8n/instance-ai/evaluations/cli/pairwise.ts b/packages/@n8n/instance-ai/evaluations/cli/pairwise.ts index 3954e52a4b0..6bc342351d2 100644 --- a/packages/@n8n/instance-ai/evaluations/cli/pairwise.ts +++ b/packages/@n8n/instance-ai/evaluations/cli/pairwise.ts @@ -65,6 +65,7 @@ interface PairwiseArgs { concurrency: number; maxExamples?: number; exampleIds?: Set; + examplesJsonl?: string; timeoutMs: number; outputDir: string; judgeModel: string; @@ -104,6 +105,7 @@ function parseArgs(argv: string[]): PairwiseArgs { parsePositiveInt(get('--concurrency'), '--concurrency') ?? Number(DEFAULTS.CONCURRENCY), maxExamples: parsePositiveInt(get('--max-examples'), '--max-examples'), exampleIds, + examplesJsonl: get('--examples-jsonl'), timeoutMs: parsePositiveNumber(get('--timeout-ms'), '--timeout-ms') ?? Number(DEFAULTS.TIMEOUT_MS), outputDir: get('--output-dir') ?? defaultOutputDir, @@ -180,6 +182,9 @@ interface DatasetExample { } async function loadExamples(args: PairwiseArgs, logger: EvalLogger): Promise { + if (args.examplesJsonl) { + return loadExamplesFromJsonl(args.examplesJsonl, logger); + } logger.info(`Fetching dataset "${args.dataset}" from LangSmith`); const lsClient = new LangSmithClient(); const examples: DatasetExample[] = []; @@ -217,6 +222,52 @@ async function loadExamples(args: PairwiseArgs, logger: EvalLogger): Promise(); + let row = 0; + for (const line of content.split('\n')) { + row++; + const trimmed = line.trim(); + if (!trimmed) continue; + let parsed: unknown; + try { + parsed = JSON.parse(trimmed); + } catch (error) { + logger.warn(`Skipping JSONL row ${row}: invalid JSON (${(error as Error).message})`); + continue; + } + if (!isRecord(parsed)) continue; + const id = typeof parsed.exampleId === 'string' ? parsed.exampleId : ''; + const prompt = typeof parsed.prompt === 'string' ? parsed.prompt : ''; + if (!id || !prompt) { + logger.warn(`Skipping JSONL row ${row}: missing exampleId or prompt`); + continue; + } + // Each iteration of the same example yields a row in results.jsonl; + // dedupe by id so we only run the example once per requested iteration. + if (seen.has(id)) continue; + seen.add(id); + examples.push({ + id, + prompt, + dos: typeof parsed.dos === 'string' ? parsed.dos : undefined, + donts: typeof parsed.donts === 'string' ? parsed.donts : undefined, + }); + } + logger.info(`Loaded ${examples.length} unique examples from ${absolute}`); + return examples; +} + // --------------------------------------------------------------------------- // Per-example runner // --------------------------------------------------------------------------- diff --git a/packages/@n8n/workflow-sdk/src/ast-interpreter/interpreter.test.ts b/packages/@n8n/workflow-sdk/src/ast-interpreter/interpreter.test.ts index 8284e18f7b6..ecf03092f63 100644 --- a/packages/@n8n/workflow-sdk/src/ast-interpreter/interpreter.test.ts +++ b/packages/@n8n/workflow-sdk/src/ast-interpreter/interpreter.test.ts @@ -56,14 +56,7 @@ const createMockSDKFunctions = (): SDKFunctions => ({ content, options, })), - placeholder: jest.fn((value: string) => ({ - __placeholder: true as const, - hint: value, - toString: () => `<__PLACEHOLDER_VALUE__${value}__>`, - toJSON() { - return this.toString(); - }, - })), + placeholder: jest.fn((value: string) => `<__PLACEHOLDER_VALUE__${value}__>`), newCredential: jest.fn((name: string) => ({ __newCredential: true, name })), ifElse: jest.fn(), switchCase: jest.fn(), @@ -970,17 +963,15 @@ describe('AST Interpreter', () => { }); }); - describe('expr(placeholder(...)) error', () => { - it('should throw clear error when expr receives a PlaceholderValue', () => { + describe('expr(placeholder(...)) round-trip', () => { + it('prepends = to the placeholder marker so it parses as an n8n expression', () => { const funcs: SDKFunctions = { ...createMockSDKFunctions(), expr, }; const code = `const val = expr(placeholder('Your ID')); export default val;`; - expect(() => interpretSDKCode(code, funcs)).toThrow( - "expr(placeholder('Your ID')) is invalid", - ); + expect(interpretSDKCode(code, funcs)).toBe('=<__PLACEHOLDER_VALUE__Your ID__>'); }); }); }); diff --git a/packages/@n8n/workflow-sdk/src/expression.test.ts b/packages/@n8n/workflow-sdk/src/expression.test.ts index dd4017276a8..6d7fef6ccca 100644 --- a/packages/@n8n/workflow-sdk/src/expression.test.ts +++ b/packages/@n8n/workflow-sdk/src/expression.test.ts @@ -30,11 +30,9 @@ describe('Expression System', () => { expect(result).toBe("={{ $('Config').item.json.apiUrl }}"); }); - it('should throw clear error when called with a PlaceholderValue', () => { - const placeholderObj = { __placeholder: true, hint: 'Your API URL' }; - expect(() => expr(placeholderObj as unknown as string)).toThrow( - "expr(placeholder('Your API URL')) is invalid. Use placeholder() directly as the value, not inside expr().", - ); + it('prepends = to a placeholder marker (preserves round-trip with `=marker`)', () => { + const marker = '<__PLACEHOLDER_VALUE__Your API URL__>'; + expect(expr(marker)).toBe('=' + marker); }); it('should throw clear error when called with a NewCredentialValue', () => { diff --git a/packages/@n8n/workflow-sdk/src/expression/index.ts b/packages/@n8n/workflow-sdk/src/expression/index.ts index ef6d49911f4..553607fc4f5 100644 --- a/packages/@n8n/workflow-sdk/src/expression/index.ts +++ b/packages/@n8n/workflow-sdk/src/expression/index.ts @@ -36,17 +36,6 @@ export function isExpression(value: unknown): boolean { * expr('={{ $json.x }}') // '={{ $json.x }}' (strips redundant =) * ``` */ -function isPlaceholderLike(value: unknown): value is { __placeholder: true; hint: string } { - return ( - typeof value === 'object' && - value !== null && - '__placeholder' in value && - (value as Record).__placeholder === true && - 'hint' in value && - typeof (value as Record).hint === 'string' - ); -} - function isNewCredentialLike(value: unknown): value is { __newCredential: true; name: string } { return ( typeof value === 'object' && @@ -60,15 +49,10 @@ function isNewCredentialLike(value: unknown): value is { __newCredential: true; export function expr(expression: string): string { if (typeof expression !== 'string') { - // At runtime, the AST interpreter may pass non-string values (e.g. PlaceholderImpl objects). + // At runtime, the AST interpreter may pass non-string values (e.g. NewCredentialImpl objects). // TypeScript narrows to `never` here since the param is typed as `string`, // so we re-bind as `unknown` to perform runtime type checks. const value: unknown = expression; - if (isPlaceholderLike(value)) { - throw new Error( - `expr(placeholder('${value.hint}')) is invalid. Use placeholder() directly as the value, not inside expr().`, - ); - } if (isNewCredentialLike(value)) { throw new Error( `expr(newCredential('${value.name}')) is invalid. Use newCredential() directly in the credentials config, not inside expr().`, diff --git a/packages/@n8n/workflow-sdk/src/generate-types/generate-types.test.ts b/packages/@n8n/workflow-sdk/src/generate-types/generate-types.test.ts index 12f2f054a95..ebfaf67bae2 100644 --- a/packages/@n8n/workflow-sdk/src/generate-types/generate-types.test.ts +++ b/packages/@n8n/workflow-sdk/src/generate-types/generate-types.test.ts @@ -365,7 +365,7 @@ describe('generate-types', () => { it('should map string type with Expression wrapper', () => { const prop: NodeProperty = { name: 'url', displayName: 'URL', type: 'string', default: '' }; const result = generateTypes.mapPropertyType(prop); - expect(result).toBe('string | Expression | PlaceholderValue'); + expect(result).toBe('string | Expression'); }); it('should map number type with Expression wrapper', () => { @@ -554,7 +554,7 @@ describe('generate-types', () => { }, }; const result = generateTypes.mapPropertyType(prop); - expect(result).toBe('Array | PlaceholderValue>'); + expect(result).toBe('Array>'); }); it('should map fixedCollection type to proper nested interface', () => { @@ -606,7 +606,7 @@ describe('generate-types', () => { ], }; const result = generateTypes.mapPropertyType(prop); - expect(result).toContain('attendees?: Array | PlaceholderValue>'); + expect(result).toContain('attendees?: Array>'); }); it('should map fixedCollection with multipleValues to array type', () => { @@ -759,7 +759,7 @@ describe('generate-types', () => { const result = generateTypes.mapPropertyType(prop); // Should generate nested structure with proper types expect(result).toContain('systemMessage?:'); - expect(result).toContain('string | Expression | PlaceholderValue'); + expect(result).toContain('string | Expression'); expect(result).toContain('maxIterations?:'); expect(result).toContain('number | Expression'); expect(result).toContain('returnIntermediateSteps?:'); @@ -845,7 +845,7 @@ describe('generate-types', () => { expect(result).toContain('@builderHint You can add multiple intervals'); }); - // PlaceholderValue tests - string type should include PlaceholderValue, other types should not + // PlaceholderValue is no longer emitted by codegen — these tests guard against regressions. it('should NOT include PlaceholderValue in options type', () => { const prop: NodeProperty = { name: 'method', @@ -2586,7 +2586,7 @@ describe('generate-types', () => { default: null, }; const result = generateTypes.mapPropertyType(prop); - expect(result).toBe('string | Expression | PlaceholderValue'); + expect(result).toBe('string | Expression'); }); it('should handle options with numeric values', () => { diff --git a/packages/@n8n/workflow-sdk/src/generate-types/generate-types.ts b/packages/@n8n/workflow-sdk/src/generate-types/generate-types.ts index aacf5c4381d..958fa617520 100644 --- a/packages/@n8n/workflow-sdk/src/generate-types/generate-types.ts +++ b/packages/@n8n/workflow-sdk/src/generate-types/generate-types.ts @@ -744,10 +744,7 @@ function mapNestedPropertyTypeInner( switch (prop.type) { case 'string': { - if (prop.builderHint?.placeholderSupported === false) { - return 'string | Expression'; - } - return 'string | Expression | PlaceholderValue'; + return 'string | Expression'; } case 'number': return 'number | Expression'; @@ -882,6 +879,12 @@ function generateNestedPropertyJSDoc( lines.push(`${indent} * @builderHint ${safeBuilderHint}`); } + // Placeholder support flag — signals to the builder agent (and the runtime + // guard) that placeholder() is rejected for this parameter. + if (prop.builderHint?.placeholderSupported === false) { + lines.push(`${indent} * @placeholderSupported false`); + } + // Search/load method annotations — signals to the builder agent that // explore-node-resources can resolve real IDs for this parameter. if (prop.modes) { @@ -1375,14 +1378,11 @@ function generateCollectionType( } /** - * Strip Expression<...> and PlaceholderValue from a type string. + * Strip Expression<...> from a type string. * Used when noDataExpression is true to produce plain types. */ function stripExpressionFromType(typeStr: string): string { - return typeStr - .replace(/\s*\|\s*Expression<[^>]+>/g, '') - .replace(/\s*\|\s*PlaceholderValue/g, '') - .trim(); + return typeStr.replace(/\s*\|\s*Expression<[^>]+>/g, '').trim(); } /** @@ -1436,10 +1436,7 @@ function mapPropertyTypeInner( switch (prop.type) { case 'string': { - if (prop.builderHint?.placeholderSupported === false) { - return 'string | Expression'; - } - return 'string | Expression | PlaceholderValue'; + return 'string | Expression'; } case 'number': @@ -1908,6 +1905,12 @@ export function generatePropertyJSDoc( lines.push(` * @builderHint ${safeBuilderHint}`); } + // Placeholder support flag — signals to the builder agent (and the runtime + // guard) that placeholder() is rejected for this parameter. + if (prop.builderHint?.placeholderSupported === false) { + lines.push(' * @placeholderSupported false'); + } + // Search/load method annotations — signals to the builder agent that // explore-node-resources can resolve real IDs for this parameter. if (prop.modes) { diff --git a/packages/@n8n/workflow-sdk/src/index.ts b/packages/@n8n/workflow-sdk/src/index.ts index 20c839d5316..8557b9fb196 100644 --- a/packages/@n8n/workflow-sdk/src/index.ts +++ b/packages/@n8n/workflow-sdk/src/index.ts @@ -41,7 +41,6 @@ export type { // Split in batches types SplitInBatchesBuilder, // Other types - PlaceholderValue, NewCredentialValue, AllItemsContext, EachItemContext, diff --git a/packages/@n8n/workflow-sdk/src/types/base.ts b/packages/@n8n/workflow-sdk/src/types/base.ts index 6d7be8cae9e..55058f19577 100644 --- a/packages/@n8n/workflow-sdk/src/types/base.ts +++ b/packages/@n8n/workflow-sdk/src/types/base.ts @@ -74,18 +74,6 @@ export interface NewCredentialValue { readonly id?: string; } -// ============================================================================= -// Placeholder Values -// ============================================================================= - -/** - * Placeholder for values the user needs to fill in. - */ -export interface PlaceholderValue { - readonly __placeholder: true; - readonly hint: string; -} - // ============================================================================= // Error Handling // ============================================================================= @@ -432,7 +420,10 @@ export interface WorkflowContext { */ export interface NodeConfig { parameters?: TParams; - credentials?: Record; + credentials?: Record< + string, + string | CredentialReference | NewCredentialValue | { value: string } + >; name?: string; position?: [number, number]; webhookId?: string; @@ -1169,7 +1160,7 @@ export type StickyFn = ( config?: StickyNoteConfig, ) => NodeInstance<'n8n-nodes-base.stickyNote', 'v1', void>; -export type PlaceholderFn = (hint: string) => PlaceholderValue; +export type PlaceholderFn = (hint: string) => string; export type NewCredentialFn = (name: string, id?: string) => NewCredentialValue; diff --git a/packages/@n8n/workflow-sdk/src/validation/index.ts b/packages/@n8n/workflow-sdk/src/validation/index.ts index 5f5c2686692..4a7a7100478 100644 --- a/packages/@n8n/workflow-sdk/src/validation/index.ts +++ b/packages/@n8n/workflow-sdk/src/validation/index.ts @@ -8,6 +8,7 @@ import { resolveMainInputCount } from './input-resolver'; import { validateNodeConfig } from './schema-validator'; import { isStickyNoteType, isHttpRequestType } from '../constants/node-types'; import type { WorkflowBuilder, WorkflowJSON } from '../types/base'; +import { containsPlaceholderMarker } from '../workflow-builder/string-utils'; export { setSchemaBaseDirs, @@ -502,6 +503,8 @@ export function validateWorkflow( validateRequiredInputsConnected(json, options.nodeTypesProvider, errors); // Validate that emitted connection types are actually exposed by the source node's mode validateOutputUsage(json, options.nodeTypesProvider, warnings); + // Reject placeholder() in slots that opt out via builderHint.placeholderSupported === false + validatePlaceholderSlots(json, options.nodeTypesProvider, errors); } // Merge node input-count consistency @@ -1016,6 +1019,58 @@ function validateOutputUsage( } } +/** + * Reject `placeholder()` markers found in parameter slots whose property + * description carries `builderHint.placeholderSupported === false`. + * + * This is the runtime side of the type-level signal that used to live in the + * generated `string | Expression` union (which previously omitted + * `PlaceholderValue`). Now that `placeholder()` returns a plain `string`, the + * type system can no longer block placement; this validator does at runtime. + * + * Uses `containsPlaceholderMarker` (not `isPlaceholderValue`) so that the + * marker is rejected anywhere in the value — including `expr(placeholder())`, + * which produces `=<__PLACEHOLDER_VALUE__…__>`, and placeholders embedded + * inside `={{ … }}` expressions. + * + * Walks top-level properties only — the known declarations + * (webhook `path`, langchain agent `text`) are top-level fields. Nested + * collection / fixedCollection support can be added later if a node opts out + * of placeholders for a nested field. + */ +function validatePlaceholderSlots( + json: WorkflowJSON, + nodeTypesProvider: INodeTypes, + errors: ValidationError[], +): void { + for (const node of json.nodes) { + if (!node.name || !node.parameters) continue; + + const version = + typeof node.typeVersion === 'string' ? parseFloat(node.typeVersion) : (node.typeVersion ?? 1); + + const nodeType = nodeTypesProvider.getByNameAndVersion(node.type, version); + const properties = nodeType?.description?.properties; + if (!properties) continue; + + const params = node.parameters as Record; + for (const prop of properties) { + if (prop.builderHint?.placeholderSupported !== false) continue; + const value = params[prop.name]; + if (!containsPlaceholderMarker(value)) continue; + + errors.push( + new ValidationError( + 'INVALID_PARAMETER', + `Node "${node.name}": placeholder() is not supported for parameter '${prop.name}'. Use a literal value or expr() instead.`, + node.name, + prop.name, + ), + ); + } + } +} + /** * Check if connections use valid input indices for their target nodes. * Reports warnings for connections to input indices that don't exist. diff --git a/packages/@n8n/workflow-sdk/src/validation/validation.test.ts b/packages/@n8n/workflow-sdk/src/validation/validation.test.ts index 862567349a2..cc16c65c7f5 100644 --- a/packages/@n8n/workflow-sdk/src/validation/validation.test.ts +++ b/packages/@n8n/workflow-sdk/src/validation/validation.test.ts @@ -2987,4 +2987,119 @@ describe('Validation', () => { expect(warnings).toHaveLength(0); }); }); + + describe('validatePlaceholderSlots (builderHint.placeholderSupported=false)', () => { + const mockNodeTypesProviderWithPlaceholderOptOut = { + getByNameAndVersion: (_type: string, _version?: number) => ({ + description: { + inputs: ['main'], + properties: [ + { + name: 'path', + displayName: 'Path', + type: 'string', + default: '', + builderHint: { placeholderSupported: false }, + }, + { + name: 'method', + displayName: 'Method', + type: 'string', + default: 'GET', + }, + ], + }, + }), + getByName: (type: string) => + mockNodeTypesProviderWithPlaceholderOptOut.getByNameAndVersion(type), + getKnownTypes: () => ({}), + }; + + const makeWorkflow = (paramValue: string) => ({ + id: 'test', + name: 'Test', + nodes: [ + { + id: 'webhook-1', + name: 'Webhook', + type: 'n8n-nodes-base.webhook', + typeVersion: 1, + position: [0, 0] as [number, number], + parameters: { path: paramValue }, + }, + ], + connections: {}, + }); + + it('rejects bare placeholder() marker', () => { + const result = validateWorkflow(makeWorkflow('<__PLACEHOLDER_VALUE__my path__>'), { + nodeTypesProvider: mockNodeTypesProviderWithPlaceholderOptOut as never, + }); + + const errors = result.errors.filter( + (e) => e.code === 'INVALID_PARAMETER' && e.message.includes('placeholder()'), + ); + expect(errors).toHaveLength(1); + expect(errors[0].nodeName).toBe('Webhook'); + expect(errors[0].parameterName).toBe('path'); + }); + + it('rejects placeholder() wrapped in expr() — leading "=" prefix', () => { + const result = validateWorkflow(makeWorkflow('=<__PLACEHOLDER_VALUE__my path__>'), { + nodeTypesProvider: mockNodeTypesProviderWithPlaceholderOptOut as never, + }); + + const errors = result.errors.filter( + (e) => e.code === 'INVALID_PARAMETER' && e.message.includes('placeholder()'), + ); + expect(errors).toHaveLength(1); + expect(errors[0].nodeName).toBe('Webhook'); + expect(errors[0].parameterName).toBe('path'); + }); + + it('rejects placeholder() embedded inside a larger expression', () => { + const result = validateWorkflow( + makeWorkflow('={{ "prefix-" + "<__PLACEHOLDER_VALUE__my path__>" }}'), + { nodeTypesProvider: mockNodeTypesProviderWithPlaceholderOptOut as never }, + ); + + const errors = result.errors.filter( + (e) => e.code === 'INVALID_PARAMETER' && e.message.includes('placeholder()'), + ); + expect(errors).toHaveLength(1); + }); + + it('does not flag literal values', () => { + const result = validateWorkflow(makeWorkflow('webhook-path'), { + nodeTypesProvider: mockNodeTypesProviderWithPlaceholderOptOut as never, + }); + + const errors = result.errors.filter( + (e) => e.code === 'INVALID_PARAMETER' && e.message.includes('placeholder()'), + ); + expect(errors).toHaveLength(0); + }); + + it('does not flag placeholder() in slots without the opt-out hint', () => { + const provider = { + getByNameAndVersion: () => ({ + description: { + inputs: ['main'], + properties: [{ name: 'path', displayName: 'Path', type: 'string', default: '' }], + }, + }), + getByName: () => provider.getByNameAndVersion(), + getKnownTypes: () => ({}), + }; + + const result = validateWorkflow(makeWorkflow('=<__PLACEHOLDER_VALUE__my path__>'), { + nodeTypesProvider: provider as never, + }); + + const errors = result.errors.filter( + (e) => e.code === 'INVALID_PARAMETER' && e.message.includes('placeholder()'), + ); + expect(errors).toHaveLength(0); + }); + }); }); diff --git a/packages/@n8n/workflow-sdk/src/workflow-builder/node-builders/node-builder.test.ts b/packages/@n8n/workflow-sdk/src/workflow-builder/node-builders/node-builder.test.ts index 5a95ae123d8..7a22d5e29b5 100644 --- a/packages/@n8n/workflow-sdk/src/workflow-builder/node-builders/node-builder.test.ts +++ b/packages/@n8n/workflow-sdk/src/workflow-builder/node-builders/node-builder.test.ts @@ -354,16 +354,15 @@ describe('Node Builder', () => { }); describe('placeholder()', () => { - it('should create a placeholder value with hint', () => { + it('returns the placeholder marker string', () => { const p = placeholder('Enter Channel'); - expect(p.__placeholder).toBe(true); - expect(p.hint).toBe('Enter Channel'); + expect(typeof p).toBe('string'); + expect(p).toBe('<__PLACEHOLDER_VALUE__Enter Channel__>'); }); - it('should serialize to placeholder format', () => { + it('JSON-serializes to the placeholder marker', () => { const p = placeholder('API Key'); - // eslint-disable-next-line @typescript-eslint/no-base-to-string -- Testing custom toString behavior - expect(String(p)).toBe('<__PLACEHOLDER_VALUE__API Key__>'); + expect(JSON.stringify({ key: p })).toBe('{"key":"<__PLACEHOLDER_VALUE__API Key__>"}'); }); }); @@ -461,8 +460,8 @@ describe('Node Builder', () => { expect((stored as { __newCredential?: boolean }).__newCredential).toBe(true); expect((stored as { name?: string }).name).toBe('Slack Bot'); expect((stored as { id?: string }).id).toBeUndefined(); - // The original __placeholder marker is gone — credentials maps never carry it. - expect((stored as { __placeholder?: boolean }).__placeholder).toBeUndefined(); + // The placeholder marker string is gone — credentials maps never carry it. + expect(typeof stored).not.toBe('string'); }); it('serializes a placeholder() credential to undefined (omitted from JSON)', () => { @@ -521,6 +520,63 @@ describe('Node Builder', () => { expect((stored as { __newCredential?: boolean }).__newCredential).toBe(true); expect((stored as { name?: string }).name).toBe('Slack Bot'); }); + + it('normalizes { value: placeholder() } shape to newCredential', () => { + const n = node({ + type: 'n8n-nodes-base.slack', + version: 2.2, + config: { credentials: { slackApi: { value: placeholder('Slack token') } } }, + }); + const stored = n.config.credentials?.slackApi as { __newCredential?: boolean; name?: string }; + expect(stored.__newCredential).toBe(true); + expect(stored.name).toBe('Slack token'); + }); + + it('normalizes { value: literal } shape to newCredential', () => { + const n = node({ + type: 'n8n-nodes-base.slack', + version: 2.2, + config: { credentials: { slackApi: { value: 'My Slack Token' } } }, + }); + const stored = n.config.credentials?.slackApi as { __newCredential?: boolean; name?: string }; + expect(stored.__newCredential).toBe(true); + expect(stored.name).toBe('My Slack Token'); + }); + + it('normalizes { id: placeholder, name: literal } to newCredential keyed by name', () => { + const n = node({ + type: 'n8n-nodes-base.slack', + version: 2.2, + config: { credentials: { slackApi: { id: placeholder('id hint'), name: 'Slack Bot' } } }, + }); + const stored = n.config.credentials?.slackApi as { __newCredential?: boolean; name?: string }; + expect(stored.__newCredential).toBe(true); + expect(stored.name).toBe('Slack Bot'); + }); + + it('normalizes { id: placeholder, name: placeholder } to newCredential keyed by name hint', () => { + const n = node({ + type: 'n8n-nodes-base.slack', + version: 2.2, + config: { + credentials: { + slackApi: { id: placeholder('id hint'), name: placeholder('Slack Bot') }, + }, + }, + }); + const stored = n.config.credentials?.slackApi as { __newCredential?: boolean; name?: string }; + expect(stored.__newCredential).toBe(true); + expect(stored.name).toBe('Slack Bot'); + }); + + it('passes a raw {id, name} CredentialReference through unchanged', () => { + const n = node({ + type: 'n8n-nodes-base.slack', + version: 2.2, + config: { credentials: { slackApi: { id: 'cred-1', name: 'Slack Bot' } } }, + }); + expect(n.config.credentials?.slackApi).toEqual({ id: 'cred-1', name: 'Slack Bot' }); + }); }); describe('then() with multiple targets (fan-out)', () => { diff --git a/packages/@n8n/workflow-sdk/src/workflow-builder/node-builders/node-builder.ts b/packages/@n8n/workflow-sdk/src/workflow-builder/node-builders/node-builder.ts index 1dad4a8c16a..a19f481e423 100644 --- a/packages/@n8n/workflow-sdk/src/workflow-builder/node-builders/node-builder.ts +++ b/packages/@n8n/workflow-sdk/src/workflow-builder/node-builders/node-builder.ts @@ -9,7 +9,6 @@ import { type NodeInput, type TriggerInput, type StickyNoteConfig, - type PlaceholderValue, type NewCredentialValue, type CredentialReference, type DeclaredConnection, @@ -21,6 +20,7 @@ import { type IfElseTarget, type SwitchCaseTarget, } from '../../types/base'; +import { extractHint, isPlaceholderValue } from '../string-utils'; import { isSwitchCaseComposite, isIfElseComposite, @@ -100,32 +100,77 @@ function generateNodeName(type: string): string { } /** - * Collapse `placeholder('hint')` markers inside a credentials map into - * `newCredential('hint')`. The two have identical intent in this slot — - * "a credential is required, no real one is bound yet" — so we normalize at - * config ingest. Downstream code (resolveCredentials, hasNewCredential, the - * `__newCredential` toJSON path) only ever sees `__newCredential` markers in - * credential slots, never `__placeholder` ones. + * Normalize the various credential-slot shapes the agent (or hand-written code) + * may emit into the canonical `CredentialReference | NewCredentialValue` form + * downstream code expects. + * + * Accepted shapes (all converge to `NewCredentialValue` or `CredentialReference`): + * - bare placeholder marker string → `newCredential(extractedHint)` + * - `{ value: }` → `newCredential()` + * - `{ id: , name: }` → `newCredential()` + * - `{ id: , name: }` → unchanged `CredentialReference` + * - `NewCredentialValue` instance → unchanged * * Returns a new config object when any normalization happens; otherwise a * shallow copy (matching the previous `{ ...config }` semantics). */ export function normalizeNodeConfig(config: NodeConfig): NodeConfig { - const creds = config?.credentials; + const creds = config?.credentials as Record | undefined; if (!creds) return { ...config }; let normalizedCreds: - | Record + | Record | undefined; + const setNormalized = (key: string, next: CredentialReference | NewCredentialValue | string) => { + normalizedCreds ??= { ...creds } as Record< + string, + CredentialReference | NewCredentialValue | string + >; + normalizedCreds[key] = next; + }; + for (const [key, value] of Object.entries(creds)) { - if (value && typeof value === 'object' && '__placeholder' in value) { - normalizedCreds ??= { ...creds }; - normalizedCreds[key] = new NewCredentialImpl(value.hint); + // 1. Bare placeholder marker string → newCredential(extractedHint) + if (typeof value === 'string' && isPlaceholderValue(value)) { + setNormalized(key, new NewCredentialImpl(extractHint(value))); + continue; } + if (!value || typeof value !== 'object') { + continue; + } + const obj = value as Record; + // Already a NewCredentialValue — leave alone + if ('__newCredential' in obj) continue; + + // 2. { value: ... } → newCredential + if ('value' in obj && !('id' in obj)) { + const v = String(obj.value); + const name = isPlaceholderValue(v) ? extractHint(v) : v; + setNormalized(key, new NewCredentialImpl(name)); + continue; + } + + // 3. { id, name } where id is a placeholder marker → newCredential(name) + if ('id' in obj) { + const idStr = typeof obj.id === 'string' ? obj.id : ''; + if (isPlaceholderValue(idStr)) { + const rawName = obj.name; + const name = + typeof rawName === 'string' && rawName.length > 0 && !isPlaceholderValue(rawName) + ? rawName + : typeof rawName === 'string' && isPlaceholderValue(rawName) + ? extractHint(rawName) + : extractHint(idStr); + setNormalized(key, new NewCredentialImpl(name)); + continue; + } + } + + // 4. else: leave as CredentialReference / plain string } if (!normalizedCreds) return { ...config }; - return { ...config, credentials: normalizedCreds }; + return { ...config, credentials: normalizedCreds } as NodeConfig; } /** @@ -1159,38 +1204,23 @@ export function sticky( } /** - * Placeholder implementation - */ -class PlaceholderImpl implements PlaceholderValue { - readonly __placeholder = true as const; - readonly hint: string; - - constructor(hint: string) { - this.hint = hint; - } - - toString(): string { - return `<__PLACEHOLDER_VALUE__${this.hint}__>`; - } - - toJSON(): string { - return this.toString(); - } -} - -/** - * Create a placeholder value for template parameters + * Create a placeholder value for template parameters. * - * Placeholders are used to mark values that need to be filled in - * when a workflow template is instantiated. + * Returns the marker string `<__PLACEHOLDER_VALUE____>`, which is + * structurally a `string` — so it flows through every string-typed slot in + * generated node types without TypeScript complaints. The workflow compiler + * recognises the marker via {@link isPlaceholderValue} and treats the slot as + * "to be filled in by the user before execution". * - * Inside a node's `credentials` slot, `placeholder(hint)` is normalized to - * `newCredential(hint)` at config ingest — the two have identical intent - * there ("a credential is required, no real one bound yet"). Outside the - * credentials slot the original placeholder semantics are unchanged. + * Inside a node's `credentials` slot, the marker is normalised to + * `newCredential(hint)` at config ingest — see {@link normalizeNodeConfig}. + * + * Note: a small number of parameters carry `placeholderSupported: false` in + * their description (e.g. webhook `path`) and the workflow compiler will throw + * a clear error if a placeholder lands in such a slot. * * @param hint - Description shown to users (e.g., 'Enter Channel') - * @returns A placeholder value that serializes to the placeholder format + * @returns The placeholder marker string. * * @example * ```typescript @@ -1200,8 +1230,8 @@ class PlaceholderImpl implements PlaceholderValue { * // Serializes channel as: '<__PLACEHOLDER_VALUE__Enter Channel__>' * ``` */ -export function placeholder(hint: string): PlaceholderValue { - return new PlaceholderImpl(hint); +export function placeholder(hint: string): string { + return `<__PLACEHOLDER_VALUE__${hint}__>`; } /** diff --git a/packages/@n8n/workflow-sdk/src/workflow-builder/pin-data-utils.test.ts b/packages/@n8n/workflow-sdk/src/workflow-builder/pin-data-utils.test.ts index a99b393ade5..eee061c10e8 100644 --- a/packages/@n8n/workflow-sdk/src/workflow-builder/pin-data-utils.test.ts +++ b/packages/@n8n/workflow-sdk/src/workflow-builder/pin-data-utils.test.ts @@ -161,12 +161,12 @@ describe('isDataTableWithoutTable', () => { expect(isDataTableWithoutTable(node)).toBe(true); }); - it('returns true for dataTable node with placeholder value', () => { + it('returns true for dataTable node with placeholder marker value', () => { const node = createNode({ type: 'n8n-nodes-base.dataTable', config: { parameters: { - dataTableId: { value: { __placeholder: true } }, + dataTableId: { value: '<__PLACEHOLDER_VALUE__Select table__>' }, }, }, }); diff --git a/packages/@n8n/workflow-sdk/src/workflow-builder/pin-data-utils.ts b/packages/@n8n/workflow-sdk/src/workflow-builder/pin-data-utils.ts index 881f889ca57..028e22859cc 100644 --- a/packages/@n8n/workflow-sdk/src/workflow-builder/pin-data-utils.ts +++ b/packages/@n8n/workflow-sdk/src/workflow-builder/pin-data-utils.ts @@ -4,6 +4,7 @@ * Functions for determining which nodes should have pin data generated. */ +import { isPlaceholderValue } from './string-utils'; import { isHttpRequestType, isWebhookType, isDataTableType } from '../constants/node-types'; import type { NodeInstance } from '../types/base'; @@ -12,11 +13,10 @@ import type { NodeInstance } from '../types/base'; * Nodes with new credentials need pin data to avoid execution errors. * * Note: by the time a NodeInstance reaches this code, credentials slots only - * ever contain `CredentialReference` or `__newCredential` markers — never - * `__placeholder` markers. `placeholder()` values supplied for credentials - * are normalized to `__newCredential` at config ingest in - * `node-builder.ts#normalizeNodeConfig`, so we don't need a second check - * here. + * ever contain `CredentialReference` or `__newCredential` markers. Bare + * placeholder marker strings and the `{ value }` convenience shape supplied + * for credentials are normalized to `__newCredential` at config ingest in + * `node-builder.ts#normalizeNodeConfig`, so we don't need a second check here. */ export function hasNewCredential(node: NodeInstance): boolean { // Check main node credentials @@ -75,12 +75,8 @@ export function isDataTableWithoutTable(node: NodeInstance = {}; for (const [key, value] of Object.entries(config.credentials)) { - if (value && typeof value === 'object' && '__placeholder' in value) continue; - resolvable[key] = value; + if (typeof value === 'string') { + if (value.startsWith('<__PLACEHOLDER_VALUE__') && value.endsWith('__>')) continue; + resolvable[key] = value as unknown as { id?: string; name: string }; + continue; + } + if (value && typeof value === 'object' && 'value' in value && !('id' in value)) continue; + resolvable[key] = value as { id?: string; name: string }; } n8nNode.credentials = deepCopy(resolvable); } diff --git a/packages/@n8n/workflow-sdk/src/workflow-builder/string-utils.test.ts b/packages/@n8n/workflow-sdk/src/workflow-builder/string-utils.test.ts index 0c4752cf569..65d91764955 100644 --- a/packages/@n8n/workflow-sdk/src/workflow-builder/string-utils.test.ts +++ b/packages/@n8n/workflow-sdk/src/workflow-builder/string-utils.test.ts @@ -5,6 +5,7 @@ import { filterMethodsFromPath, parseVersion, isPlaceholderValue, + containsPlaceholderMarker, isResourceLocatorLike, normalizeResourceLocators, escapeNewlinesInStringLiterals, @@ -106,6 +107,38 @@ describe('workflow-builder/string-utils', () => { }); }); + describe('containsPlaceholderMarker', () => { + it('returns true for bare placeholder string', () => { + expect(containsPlaceholderMarker('<__PLACEHOLDER_VALUE__test__>')).toBe(true); + }); + + it('returns true for expr()-wrapped placeholder (leading "=")', () => { + expect(containsPlaceholderMarker('=<__PLACEHOLDER_VALUE__test__>')).toBe(true); + }); + + it('returns true for placeholder embedded in an expression block', () => { + expect(containsPlaceholderMarker('={{ "prefix-" + "<__PLACEHOLDER_VALUE__name__>" }}')).toBe( + true, + ); + }); + + it('returns false for literal strings without the marker', () => { + expect(containsPlaceholderMarker('regular string')).toBe(false); + expect(containsPlaceholderMarker('=expression')).toBe(false); + }); + + it('returns false for incomplete marker (missing closing)', () => { + expect(containsPlaceholderMarker('<__PLACEHOLDER_VALUE__test')).toBe(false); + }); + + it('returns false for non-string values', () => { + expect(containsPlaceholderMarker(123)).toBe(false); + expect(containsPlaceholderMarker(null)).toBe(false); + expect(containsPlaceholderMarker(undefined)).toBe(false); + expect(containsPlaceholderMarker({})).toBe(false); + }); + }); + describe('isResourceLocatorLike', () => { it('returns true for object with mode and value', () => { expect(isResourceLocatorLike({ mode: 'list', value: 'test' })).toBe(true); diff --git a/packages/@n8n/workflow-sdk/src/workflow-builder/string-utils.ts b/packages/@n8n/workflow-sdk/src/workflow-builder/string-utils.ts index d8243fbbf4d..7f3e4cac05a 100644 --- a/packages/@n8n/workflow-sdk/src/workflow-builder/string-utils.ts +++ b/packages/@n8n/workflow-sdk/src/workflow-builder/string-utils.ts @@ -68,6 +68,31 @@ export function isPlaceholderValue(value: unknown): boolean { return value.startsWith('<__PLACEHOLDER_VALUE__') && value.endsWith('__>'); } +/** + * Check if a value contains a placeholder marker anywhere within it. + * + * Unlike {@link isPlaceholderValue}, which requires the marker to span the + * entire string, this catches placeholders embedded in larger strings — most + * notably `=<__PLACEHOLDER_VALUE__…__>` produced by wrapping `placeholder()` + * inside `expr()`, or placeholders concatenated inside `={{ … }}` blocks. + */ +export function containsPlaceholderMarker(value: unknown): boolean { + if (typeof value !== 'string') return false; + return /<__PLACEHOLDER_VALUE__[\s\S]*?__>/.test(value); +} + +/** + * Extract the original hint from a placeholder marker string. + * Returns the input unchanged if it does not match the marker format. + * + * @example + * extractHint('<__PLACEHOLDER_VALUE__OpenAI key__>') // → 'OpenAI key' + */ +export function extractHint(value: string): string { + if (!isPlaceholderValue(value)) return value; + return value.slice('<__PLACEHOLDER_VALUE__'.length, -'__>'.length); +} + /** * Check if an object looks like a resource locator value. * Resource locators have a 'mode' property (typically 'list', 'id', 'url', or 'name') diff --git a/packages/@n8n/workflow-sdk/src/workflow-builder/validation-helpers.test.ts b/packages/@n8n/workflow-sdk/src/workflow-builder/validation-helpers.test.ts index cdd88e0289f..a252c4d415c 100644 --- a/packages/@n8n/workflow-sdk/src/workflow-builder/validation-helpers.test.ts +++ b/packages/@n8n/workflow-sdk/src/workflow-builder/validation-helpers.test.ts @@ -194,12 +194,9 @@ describe('workflow-builder/validation-helpers', () => { expect(issues[0].path).toBe('items[1]'); }); - it('skips PlaceholderValue objects', () => { + it('skips placeholder marker strings', () => { const issues = findMissingExpressionPrefixes({ - field: { - __placeholder: true, - hint: '{{ $json.field }}', - }, + field: '<__PLACEHOLDER_VALUE__{{ $json.field }}__>', }); expect(issues).toHaveLength(0); }); diff --git a/packages/@n8n/workflow-sdk/src/workflow-builder/validation-helpers.ts b/packages/@n8n/workflow-sdk/src/workflow-builder/validation-helpers.ts index 6525da73adc..81729fe24f5 100644 --- a/packages/@n8n/workflow-sdk/src/workflow-builder/validation-helpers.ts +++ b/packages/@n8n/workflow-sdk/src/workflow-builder/validation-helpers.ts @@ -3,6 +3,7 @@ * Pure functions extracted from WorkflowBuilderImpl */ +import { isPlaceholderValue } from './string-utils'; import { isTriggerNodeType } from '../utils/trigger-detection'; /** @@ -124,6 +125,8 @@ export function findMissingExpressionPrefixes( const issues: Array<{ path: string; value: string }> = []; if (typeof value === 'string') { + // Skip placeholder markers — their embedded hint is documentation, not an expression. + if (isPlaceholderValue(value)) return issues; // If string starts with '=', it's already an expression - {{ }} is valid template syntax inside // Otherwise check if it contains {{ $ pattern (n8n variable reference without = prefix) if (!value.startsWith('=') && value.includes('{{ $')) { @@ -134,10 +137,6 @@ export function findMissingExpressionPrefixes( issues.push(...findMissingExpressionPrefixes(item, `${path}[${index}]`)); }); } else if (value && typeof value === 'object') { - // Skip PlaceholderValue objects - their hint property is documentation, not actual expressions - if ('__placeholder' in value && (value as { __placeholder: boolean }).__placeholder) { - return issues; - } for (const [key, val] of Object.entries(value)) { const newPath = path ? `${path}.${key}` : key; issues.push(...findMissingExpressionPrefixes(val, newPath)); @@ -181,6 +180,7 @@ export function findInvalidDateMethods( const issues: Array<{ path: string; value: string }> = []; if (typeof value === 'string') { + if (isPlaceholderValue(value)) return issues; if (hasLuxonToISOStringMisuse(value)) { issues.push({ path, value }); } @@ -189,10 +189,6 @@ export function findInvalidDateMethods( issues.push(...findInvalidDateMethods(item, `${path}[${index}]`)); }); } else if (value && typeof value === 'object') { - // Skip PlaceholderValue objects - if ('__placeholder' in value && (value as { __placeholder: boolean }).__placeholder) { - return issues; - } for (const [key, val] of Object.entries(value)) { const newPath = path ? `${path}.${key}` : key; issues.push(...findInvalidDateMethods(val, newPath));