feat: add modify existing workflow action and output schema validation #14
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces a workflow modification system with schema-driven field validation and auto-correction. It adds new scripts for generating node and trigger schema catalogs, expands validation utilities for output references and parameter correction, and integrates these capabilities into the workflow generation pipeline. Includes comprehensive test coverage and GitHub Actions workflows for automated schema updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ModifyAction as Modify Action
participant N8nService as N8N Workflow Service
participant N8nAPI as N8N API
participant Cache as Draft Cache
User->>ModifyAction: Request to modify workflow
ModifyAction->>Cache: Check for existing draft
Cache-->>ModifyAction: Draft found or none
alt Draft exists and not expired
ModifyAction-->>User: Prompt to finish/cancel current draft
else No draft
ModifyAction->>N8nService: List user workflows
N8nService->>N8nAPI: GET /workflows
N8nAPI-->>N8nService: Workflows list
N8nService-->>ModifyAction: Deployed workflows
ModifyAction->>ModifyAction: Match user description to workflow
alt Match confidence: high/medium
ModifyAction->>N8nAPI: GET /workflows/:id
N8nAPI-->>ModifyAction: Workflow details
ModifyAction->>Cache: Store draft with originMessageId
ModifyAction-->>User: Return preview with workflow details
else Match confidence: low
ModifyAction-->>User: Request confirmation
else No match
ModifyAction-->>User: Return workflow list for disambiguation
end
end
sequenceDiagram
participant Generator as Workflow Generator
participant Validator as Validation Engine
participant Corrector as Correction Service
participant LLM as Language Model
participant Catalog as Node Catalog
Generator->>Generator: Generate initial workflow
Generator->>Validator: validateWorkflow()
Validator->>Validator: Check structure, connections
Validator-->>Generator: Validation results
Generator->>Validator: validateOutputReferences()
Validator->>Catalog: Load upstream output schemas
Catalog-->>Validator: Field definitions
Validator->>Validator: Check $json expressions match fields
Validator-->>Generator: Invalid references list
alt Invalid references exist
Generator->>Corrector: correctFieldReferences()
Corrector->>LLM: Request field path corrections
LLM-->>Corrector: Corrected expressions
Corrector->>Generator: Updated workflow
end
Generator->>Validator: detectUnknownParameters()
Validator->>Catalog: Get property definitions
Catalog-->>Validator: Known properties
Validator-->>Generator: Unknown parameters list
alt Unknown parameters exist
Generator->>Corrector: correctParameterNames()
Corrector->>LLM: Request parameter mapping
LLM-->>Corrector: Corrected names
Corrector->>Generator: Updated workflow
end
Generator->>Validator: ensureExpressionPrefix()
Validator->>Validator: Prefix $json expressions with "="
Validator-->>Generator: Prefixed workflow
Generator-->>Generator: Return validated, corrected workflow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment was marked as outdated.
This comment was marked as outdated.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
standujar
force-pushed
the
feat/workflow-modify-and-schema-validation
branch
from
5e6c29c to
ee307ef
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
standujar
force-pushed
the
feat/workflow-modify-and-schema-validation
branch
from
ee307ef to
7057f70
Compare
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/actions/modifyExistingWorkflow.ts`:
- Around line 127-138: modifyExistingWorkflow skips using expired WorkflowDrafts
but doesn't remove them from cache, allowing stale entries to accumulate; fix by
deleting the expired cache entry. In the modifyExistingWorkflow flow where you
call runtime.getCache<WorkflowDraft>(cacheKey) and detect an expired draft via
DRAFT_TTL_MS, add await runtime.deleteCache(cacheKey) before
returning/continuing (mirroring createWorkflow/activateWorkflow behavior);
ensure you keep the existing callback handling and return { success: false }
after deletion to avoid changing control flow.
In `@src/utils/generation.ts`:
- Around line 468-482: The replaceInObject traversal currently uses
String.replace which only replaces the first occurrence; update the
string-replacement logic in replaceInObject (for obj[key] and array element
value[i]) to replace all occurrences of original by replacement (e.g., use
value.replaceAll(original, replacement) or value.replace(new
RegExp(escapeForRegExp(original), 'g'), replacement) or
value.split(original).join(replacement)), ensuring you handle possible
special-regex characters when choosing the RegExp approach.
In `@src/utils/outputSchema.ts`:
- Around line 234-278: The function fieldExistsInSchema can exit the loop after
consuming an array index (the i++ in the array branch) and then fall through to
the final `return false`; to fix it, inside the `else if (prop.type === 'array'
&& prop.items)` branch (in fieldExistsInSchema) after incrementing `i` and
setting `current = prop.items`, add a check that if `i === path.length - 1`
return true (since the numeric index was the last segment), ensuring the
function returns true when the final segment is reached via an array items
traversal.
- Around line 1-6: Change the legacy JSON import assertion on the schemaIndex
import to the modern `with` form: locate the import statement that brings in
schemaIndex (import schemaIndex from '../data/schemaIndex.json' assert { type:
'json' };) in src/utils/outputSchema.ts and replace the `assert { type: 'json'
}` usage with the standardized `with { type: 'json' }` syntax so the module
imports correctly under TypeScript 5.3+ / Node 22+.
In `@src/utils/workflow.ts`:
- Around line 301-347: The current validateOutputReferences only checks the
first upstream node; change it to validate each expression against all
applicable upstreams: for each node in workflow.nodes get expressions via
parseExpressions and upstreamNames from buildUpstreamMap, then for each
expression iterate upstreamNames (or the explicitly referenced node if the
expression targets $('NodeName')—handle that case first) and for each upstream
load the schema via loadOutputSchema and test with fieldExistsInSchema; treat
the expression as valid if any upstream schema contains the field, and only push
an invalid ref when none of the upstream schemas match, providing
availableFields as the union (or concatenation) of getAllFieldPaths results from
all attempted upstream schemas; update references to sourceNode/sourceNodeName
accordingly.
🧹 Nitpick comments (7)
scripts/crawl-schemas.ts (2)
15-19: Consider aligningSchemaContentwith the runtime type definition.The
SchemaContentinterface here is missing theitemsproperty that exists insrc/types/index.ts:352-357. While this script generates the schema index and may not need the full type, keeping them consistent prevents potential mismatches.♻️ Suggested alignment
interface SchemaContent { type: string; properties?: Record<string, unknown>; + items?: SchemaContent; [key: string]: unknown; }
93-100: Consider logging skipped files for debugging purposes.The empty catch blocks silently swallow errors, which is fine for production robustness but can make debugging difficult when schema files are unexpectedly missing from the index.
♻️ Optional: Add debug logging
try { const schemaPath = path.join(resourcePath, opFile.name); const content = await readFile(schemaPath, 'utf-8'); const schema = JSON.parse(content) as SchemaContent; schemas[resource][operation] = schema; } catch { - // Skip invalid schema files + // Skip invalid schema files (uncomment for debugging) + // console.warn(`Skipping invalid schema: ${path.join(resourcePath, opFile.name)}`); }src/utils/credentialResolver.ts (1)
111-116: Consider sanitizingtagNamefor credential name.The credential name
${credType}_${tagName}could contain problematic characters iftagNameincludes spaces, special characters, or is excessively long. Depending on n8n's credential name constraints, this could cause API errors.♻️ Optional: Sanitize tagName before use
- const credName = `${credType}_${tagName}`; + const sanitizedTag = tagName.replace(/[^a-zA-Z0-9_:-]/g, '_').slice(0, 50); + const credName = `${credType}_${sanitizedTag}`;src/actions/createWorkflow.ts (1)
88-112: Parameter diff only captures changed/added parameters, not removed ones.The current implementation detects parameters that changed or were added in
afterNode, but won't report parameters that existed inbeforeNodebut were removed inafterNode. This may be intentional for preview purposes, but worth noting.Additionally,
JSON.stringifycomparison is order-sensitive for object properties, which could produce false positives if the LLM reorders keys without changing values.♻️ Optional: Use deep equality and track removed params
function diffNodeParams( before: N8nWorkflow, after: N8nWorkflow ): Record<string, Record<string, unknown>> { const changes: Record<string, Record<string, unknown>> = {}; for (const afterNode of after.nodes) { const beforeNode = before.nodes.find((n) => n.name === afterNode.name); const afterParams = (afterNode.parameters || {}) as Record<string, unknown>; const beforeParams = (beforeNode?.parameters || {}) as Record<string, unknown>; const nodeChanges: Record<string, unknown> = {}; + const allKeys = new Set([...Object.keys(afterParams), ...Object.keys(beforeParams)]); - for (const [key, value] of Object.entries(afterParams)) { - if (JSON.stringify(value) !== JSON.stringify(beforeParams[key])) { - nodeChanges[key] = value; - } - } + for (const key of allKeys) { + const beforeVal = beforeParams[key]; + const afterVal = afterParams[key]; + if (JSON.stringify(beforeVal) !== JSON.stringify(afterVal)) { + nodeChanges[key] = afterVal; // undefined if removed + } + } if (Object.keys(nodeChanges).length > 0) { changes[afterNode.name] = nodeChanges; } } return changes; }__tests__/integration/actions/createWorkflow.test.ts (1)
521-554: DuplicatecreateDraftInCachehelper function.This function is identical to the one defined at lines 187-220. Consider extracting it to a shared location within the test file or to test fixtures to avoid duplication.
♻️ Move shared helper to top of describe block or fixtures
+// Move createDraftInCache to top-level of the describe block, outside nested describes describe('CREATE_N8N_WORKFLOW action', () => { + function createDraftInCache(): WorkflowDraft { + return { + workflow: { + name: 'Stripe Gmail Summary', + // ... rest of implementation + }, + prompt: 'Send Stripe summaries via Gmail', + userId: 'user-001', + createdAt: Date.now(), + }; + } + describe('handler - existing draft', () => { - function createDraftInCache(): WorkflowDraft { ... } // ... tests }); describe('handler - explicit intent via _options', () => { - function createDraftInCache(): WorkflowDraft { ... } // ... tests }); });src/services/n8n-workflow-service.ts (1)
243-250: Consider extracting the duplicated validation/correction pattern.The same validation and auto-correction logic is repeated in both
generateWorkflowDraftandmodifyWorkflowDraft. This could be extracted into a private helper method to reduce duplication.♻️ Optional refactor to extract helper
+ private async validateAndCorrectReferences(workflow: N8nWorkflow): Promise<N8nWorkflow> { + const invalidRefs = validateOutputReferences(workflow); + if (invalidRefs.length > 0) { + logger.debug( + { src: 'plugin:n8n-workflow:service:main' }, + `Found ${invalidRefs.length} invalid field reference(s), auto-correcting...` + ); + return correctFieldReferences(this.runtime, workflow, invalidRefs); + } + return workflow; + }Then in both methods:
- const invalidRefs = validateOutputReferences(workflow); - if (invalidRefs.length > 0) { - logger.debug( - { src: 'plugin:n8n-workflow:service:main' }, - `Found ${invalidRefs.length} invalid field reference(s), auto-correcting...` - ); - workflow = await correctFieldReferences(this.runtime, workflow, invalidRefs); - } + workflow = await this.validateAndCorrectReferences(workflow);Also applies to: 310-317
src/utils/outputSchema.ts (1)
86-115: Consider adding a depth limit to prevent stack overflow on deeply nested schemas.
getAllFieldPathsrecursively traverses nested objects and arrays without a depth limit. While typical n8n schemas are shallow, malformed or adversarial input could cause stack overflow.♻️ Optional: Add depth protection
-export function getAllFieldPaths(schema: SchemaContent, prefix = ''): string[] { +export function getAllFieldPaths(schema: SchemaContent, prefix = '', maxDepth = 10): string[] { const paths: string[] = []; const properties = schema.properties; - if (!properties) { + if (!properties || maxDepth <= 0) { return paths; } for (const [key, value] of Object.entries(properties)) { const currentPath = prefix ? `${prefix}.${key}` : key; paths.push(currentPath); if (typeof value === 'object' && value !== null) { const propSchema = value as SchemaContent; if (propSchema.type === 'object' && propSchema.properties) { - paths.push(...getAllFieldPaths(propSchema, currentPath)); + paths.push(...getAllFieldPaths(propSchema, currentPath, maxDepth - 1)); } if (propSchema.type === 'array' && propSchema.items) { const items = propSchema.items as SchemaContent; if (items.type === 'object' && items.properties) { - paths.push(...getAllFieldPaths(items, `${currentPath}[0]`)); + paths.push(...getAllFieldPaths(items, `${currentPath}[0]`, maxDepth - 1)); } } } } return paths; }
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as abuse.
This comment was marked as abuse.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/actions/modifyExistingWorkflow.ts`:
- Around line 122-142: The code assumes message.entityId exists before calling
service.listWorkflows, which lets listWorkflows(undefined) return unfiltered
results; add a guard that validates message.entityId / userId is present and
return an error (via callback and { success: false }) before checking cache or
calling service.listWorkflows. Specifically, in modifyExistingWorkflow.ts check
message.entityId (used as userId) at the top of the try block and short-circuit
with a callback error and return when it's falsy, ensuring
runtime.getCache(cacheKey) and service.listWorkflows(userId) only run with a
valid userId; apply the same pattern to the other files mentioned.
In `@src/services/n8n-workflow-service.ts`:
- Around line 243-250: After calling correctFieldReferences, re-run
validateOutputReferences against the updated workflow and handle any remaining
invalidRefs: call validateOutputReferences(workflow) again, and if the returned
list is non-empty, surface the issue (e.g., logger.warn or throw a clear error
requiring clarification) rather than proceeding; update the block around
validateOutputReferences / correctFieldReferences (referencing
validateOutputReferences, correctFieldReferences, this.runtime, and workflow) to
perform this second validation and decide whether to warn or halt when residual
invalid references remain.
🧹 Nitpick comments (1)
__tests__/integration/actions/createWorkflow.test.ts (1)
521-554: Duplicate helper functioncreateDraftInCache— consider extracting to shared scope.This helper is identical to the one defined at lines 187-220 in the "handler - existing draft" describe block. Consider hoisting it to the top-level describe scope to eliminate duplication.
♻️ Suggested refactor
Move
createDraftInCacheto the top-leveldescribe('CREATE_N8N_WORKFLOW action', ...)block so both test suites can share it:describe('CREATE_N8N_WORKFLOW action', () => { + function createDraftInCache(): WorkflowDraft { + return { + workflow: { + name: 'Stripe Gmail Summary', + // ... rest of implementation + }, + prompt: 'Send Stripe summaries via Gmail', + userId: 'user-001', + createdAt: Date.now(), + }; + } + describe('validate', () => { // ... }); describe('handler - existing draft', () => { - function createDraftInCache(): WorkflowDraft { ... } // tests using createDraftInCache() }); describe('handler - explicit intent via _options', () => { - function createDraftInCache(): WorkflowDraft { ... } // tests using createDraftInCache() });
- Add MODIFY_EXISTING_N8N_WORKFLOW action to load deployed workflows for editing - Add output schema validation to check $json expressions against upstream node schemas - Add auto-correction for invalid field references using LLM - Add explicit intent support via _options for multi-step agent control - Include parameter diff in preview response after workflow modification - Preserve workflow ID through modification flow for updates (not creates) - Add crawl-schemas script to extract output schemas from n8n-nodes-base
standujar
force-pushed
the
feat/workflow-modify-and-schema-validation
branch
from
7057f70 to
e7bddc0
Compare
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
…e hardening - Add trigger output schema capture and validation (Gmail, GitHub, Google Calendar, etc.) - Fix parseExpressions regex to detect $json refs in compound expressions (||, ternary) - Include field types in correction prompt so LLM picks string over object - Force simple=true on trigger nodes that support it (normalizeTriggerSimpleParam) - Skip schema validation when simple=false (raw mode differs from captured schema) - Remove explicit intent parameter to prevent multi-step agent auto-confirm - Add originMessageId guard to block same-turn draft confirmation - Remove duplicate intent classification log - Add schema-update CI workflow (weekly cron, auto-bump on schema change) - Update npm-deploy workflow with crawl + trigger capture steps
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/actions/createWorkflow.ts (1)
396-437:⚠️ Potential issue | 🟡 MinorModified draft doesn't carry
originMessageId— intentional?On line 408, the modified draft is created without
originMessageId. This means if the framework auto-chains anotherCREATE_N8N_WORKFLOWcall in the same turn after a modify, the same-turn guard (line 316) won't fire. ThegenerateAndPreviewpath (line 562) does set it. Consider settingoriginMessageId: message.idhere for consistency.Proposed fix
const modifiedDraft: WorkflowDraft = { workflow: modifiedWorkflow, prompt: existingDraft.prompt, userId, createdAt: Date.now(), + originMessageId: message.id, };
🤖 Fix all issues with AI agents
In @.github/workflows/npm-deploy.yml:
- Around line 76-80: The "Capture trigger schemas" workflow step currently can
fail the whole publish pipeline if N8N_HOST or secrets are misconfigured and it
also uses an unrecognized flag; update the step named "Capture trigger schemas"
to add continue-on-error: true so schema capture failures don't block
publishing, and remove (or correct) the unsupported "--from-existing" argument
passed to the script invocation ("bun run scripts/capture-trigger-schemas.ts")
so the step calls the script with only supported flags.
In @.github/workflows/schema-update.yml:
- Around line 22-23: The CI flag --from-existing is not parsed by
capture-trigger-schemas.ts so it is ignored; update the CLI parsing (where
--trigger, --timeout, --keep, and --create-only are handled) to accept a
--from-existing boolean (e.g., set a fromExisting variable in parseArgs or the
existing argument-parsing block) and then propagate that flag into the main flow
(functions like captureTriggerSchemas / createAndActivateWorkflows or whichever
routines perform creation/activation) so that when fromExisting is true the code
only reads existing workflows and explicitly skips any creation or activation
steps.
In `@scripts/capture-trigger-schemas.ts`:
- Around line 135-143: The KNOWN_CREDENTIALS constant currently contains
hardcoded credential IDs; remove the hardcoded map and load these IDs from
environment variables instead (either a single JSON string env like
KNOWN_CREDENTIALS_JSON that you JSON.parse and validate, or individual env vars
per key such as KNOWN_CREDENTIALS_gmailOAuth2, _googleCalendarOAuth2Api, etc.).
Update scripts/capture-trigger-schemas.ts to read process.env for these values,
validate required keys exist (throw or log a clear error if missing), and
fallback to an empty object only for non-critical keys; ensure no secret values
remain in source control and document the expected env var names in the project
README or .env.example.
- Around line 305-310: The script's main() argument parsing misses the
--from-existing flag, so add parsing for it (e.g., const fromExisting =
args.includes('--from-existing')) alongside existing flags (filterTrigger,
timeoutSec, keepWorkflows, createOnly) and then use that boolean to
short-circuit the create/activate/wait flow in the trigger capture logic (the
block that currently creates, activates, and waits for triggers between lines
84-132), making the script only capture schemas from existing workflows when
fromExisting is true; update any downstream conditionals that currently check
createOnly/keepWorkflows to also respect fromExisting where appropriate.
In `@scripts/crawl-trigger-schemas.ts`:
- Around line 136-146: Check response.ok before calling response.json() and only
parse and write to disk when the HTTP response is successful: after fetching
(the response variable) if !response.ok throw or handle the error (do not call
response.json or update cachedGuruIndex), and ensure fs.writeFileSync(cachePath,
...) and the console.log that reports cached APIs run only when cachedGuruIndex
was successfully populated; update the logic around cachedGuruIndex, response,
CACHE_DIR and cachePath so failed responses are not parsed or written to the
cache and return or propagate an error instead.
In `@scripts/create-credentials.ts`:
- Line 1: The shebang in scripts/create-credentials.ts currently uses
npx/ts-node which conflicts with the project's Bun runtime; either replace the
top-line shebang with one that invokes Bun (#!/usr/bin/env bun) if you intend
the script to be directly executable, or remove the shebang entirely if the
script will be run via bun run (e.g., package.json scripts); update the file's
first line accordingly so the runtime matches how create-credentials.ts is
executed.
- Around line 168-180: The current credential creation pushes a Twitter entry
into configs with empty-string fallbacks for TWITTER_API_SECRET and
TWITTER_ACCESS_TOKEN_SECRET (and similarly uses '' for GITHUB_USER), which
creates invalid creds; update the guard that builds the twitter credential (the
block that pushes the object with name '[Auto] Twitter OAuth1' and type
'twitterOAuth1Api') to require all four env vars (TWITTER_API_KEY,
TWITTER_API_SECRET, TWITTER_ACCESS_TOKEN, TWITTER_ACCESS_TOKEN_SECRET) before
pushing, or skip creation and emit a warning via logger when any required secret
is missing; likewise remove the empty-string fallback for GITHUB_USER and only
create the GitHub credential when the required env var(s) are present, logging a
clear warning if partial env data is provided.
In `@src/actions/modifyExistingWorkflow.ts`:
- Around line 222-229: The call to formatActionResponse(runtime,
'WORKFLOW_LOADED', ...) uses an undefined response type —
ACTION_RESPONSE_SYSTEM_PROMPT only contains PREVIEW, CLARIFICATION,
DEPLOY_SUCCESS, AUTH_REQUIRED, CANCELLED, EMPTY_PROMPT, UNSUPPORTED_INTEGRATION,
and ERROR — so either change the second argument in modifyExistingWorkflow (the
formatActionResponse call) to an existing type like 'PREVIEW' or add a new
'WORKFLOW_LOADED' entry to the ACTION_RESPONSE_SYSTEM_PROMPT definition in
src/prompts/actionResponse.ts; update the prompt text for WORKFLOW_LOADED if
adding it so it matches the intended output format and keep references to
formatActionResponse and ACTION_RESPONSE_SYSTEM_PROMPT consistent.
- Around line 198-205: The draft created in modifyExistingWorkflow.ts is missing
the WorkflowDraft.originMessageId field so the same-turn auto-confirm guard
(which checks existingDraft.originMessageId === message.id in createWorkflow.ts)
can be bypassed; fix by adding originMessageId: message.id (or the appropriate
incoming message identifier variable available in this scope) to the draft
object before calling runtime.setCache(cacheKey, draft) so the guard will
correctly detect same-turn drafts.
In `@src/utils/generation.ts`:
- Around line 409-433: The LLM output (variable cleaned) is used blindly and may
not be a valid expression; add a lightweight validation after computing cleaned
(in the async map over invalidRefs that produces corrections) that checks
cleaned against expected patterns (eg. matches /^\$json\.[\w\.]+$/ or
/^\$\(.+\)$/ or other allowed prefixes like "$json." or "$("), and only return {
original, corrected, nodeName } when it passes; otherwise logger.warn with
ref.expression and cleaned and return null so invalid LLM responses are not
applied. Ensure the validation logic and logging occur in the same try block
right after the cleaned assignment (referencing cleaned, ref.expression, and
nodeName).
🧹 Nitpick comments (9)
src/utils/credentialResolver.ts (1)
69-78: Consider using an options object to reduce positional parameter sprawl.
resolveOneCredentialnow has 8 positional parameters, making call sites fragile and harder to read. An options/config object would improve clarity and make future additions less error-prone.♻️ Example refactor
-async function resolveOneCredential( - credType: string, - userId: string, - config: N8nPluginConfig, - credStore: N8nCredentialStoreApi | null, - credProvider: CredentialProvider | null, - apiClient: N8nApiClient | null, - missingConnections: MissingConnection[], - tagName: string -): Promise<string | null> { +interface ResolveOneCredentialOpts { + credType: string; + userId: string; + config: N8nPluginConfig; + credStore: N8nCredentialStoreApi | null; + credProvider: CredentialProvider | null; + apiClient: N8nApiClient | null; + missingConnections: MissingConnection[]; + tagName: string; +} + +async function resolveOneCredential(opts: ResolveOneCredentialOpts): Promise<string | null> { + const { credType, userId, config, credStore, credProvider, apiClient, missingConnections, tagName } = opts;scripts/create-credentials.ts (1)
53-61:JSON.parseon potentially malformed response text will throw an opaque error.If the n8n API returns a non-204 success status with malformed JSON,
JSON.parse(text)on line 60 will throw aSyntaxErrorwithout context about which endpoint failed. A try-catch with the endpoint info would make debugging easier.Proposed improvement
const text = await response.text(); - return text ? JSON.parse(text) : undefined; + if (!text) return undefined as T; + try { + return JSON.parse(text) as T; + } catch { + throw new Error(`n8n API ${method} ${endpoint}: invalid JSON response: ${text.slice(0, 200)}`); + }scripts/crawl-trigger-schemas.ts (2)
415-432: Dead code: then8n === 'me'guard on Line 427 is unreachable.Line 425 already handles
spec.startsWith('{')withcontinue, sospec.startsWith('{')on Line 427 is alwaysfalse. The branch is harmless but misleading.♻️ Remove or consolidate
// Template params match anything if (spec.startsWith('{') || n8n.startsWith('{')) continue; - // "me" matches "{userId}" equivalent - if (n8n === 'me' && spec.startsWith('{')) continue; if (n8n !== spec) return false;
751-771: Unhandled rejection fromcrawlTriggers()will produce a cryptic error.Add a
.catch()handler (like the companion scriptcapture-trigger-schemas.tsdoes) for clearer failure output.♻️ Suggested fix
-crawlTriggers().then((result) => { +crawlTriggers().then((result) => { // ... existing code ... -}); +}).catch((error) => { + console.error('Fatal:', error); + process.exit(1); +});.github/workflows/schema-update.yml (1)
30-34: Verify the tarball extraction path convention for scoped npm packages.The glob
elizaos-plugin-n8n-workflow-*.tgzassumes npm's scoped-package tarball naming. If the package scope or name ever changes, this will silently break (no file found → compare step skips everything → no update triggered). Consider failing explicitly if no tarball is found.🛡️ Suggested guard
mkdir -p /tmp/latest bunx npm pack `@elizaos/plugin-n8n-workflow`@latest --pack-destination /tmp - tar xzf /tmp/elizaos-plugin-n8n-workflow-*.tgz -C /tmp/latest + TARBALL=$(ls /tmp/elizaos-plugin-n8n-workflow-*.tgz 2>/dev/null | head -1) + if [ -z "$TARBALL" ]; then + echo "::warning::No tarball found — first publish?" + else + tar xzf "$TARBALL" -C /tmp/latest + fisrc/utils/workflow.ts (1)
294-307:normalizeTriggerSimpleParamsilently overrides explicitsimple: false.If a user intentionally configured full (non-simplified) trigger output, this function forces it back to
true. This is likely intended to align with schema validation (which only covers simplified output), but it would be worth a log when overriding an explicit value.♻️ Suggested improvement
if (hasSimple) { + if (node.parameters?.simple === false) { + // Log when overriding an explicit user choice + // (schema validation requires simplified output) + } node.parameters = { ...node.parameters, simple: true }; }.github/workflows/npm-deploy.yml (1)
98-98: Migrateactions/create-release@v1to an actively maintained alternative.This action was archived on March 4, 2021 and is now read-only. It will not receive bug fixes, security patches, or updates. Use
softprops/action-gh-release@v2or thegh release createCLI command instead.__tests__/unit/workflow.test.ts (1)
414-440: Consider adding a test for a node with all required parameters satisfied.Currently only the negative case (missing required param) and the skip-unknown case are tested. A positive test confirming zero warnings for a fully specified node would strengthen the suite and guard against regressions.
src/actions/createWorkflow.ts (1)
88-112:diffNodeParamsonly detects changed/added params — removed nodes and removed params are invisible.If a modification removes a node or deletes a parameter, it won't appear in the diff. For preview purposes this is acceptable since the full node list is shown, but it could confuse users who expect the diff to be exhaustive.
Optional: also surface removed params
function diffNodeParams( before: N8nWorkflow, after: N8nWorkflow ): Record<string, Record<string, unknown>> { const changes: Record<string, Record<string, unknown>> = {}; for (const afterNode of after.nodes) { const beforeNode = before.nodes.find((n) => n.name === afterNode.name); const afterParams = (afterNode.parameters || {}) as Record<string, unknown>; const beforeParams = (beforeNode?.parameters || {}) as Record<string, unknown>; const nodeChanges: Record<string, unknown> = {}; for (const [key, value] of Object.entries(afterParams)) { if (JSON.stringify(value) !== JSON.stringify(beforeParams[key])) { nodeChanges[key] = value; } } + // Detect removed params + for (const key of Object.keys(beforeParams)) { + if (!(key in afterParams)) { + nodeChanges[key] = null; // signals removal + } + } if (Object.keys(nodeChanges).length > 0) { changes[afterNode.name] = nodeChanges; } } return changes; }
.github/workflows/schema-update.yml
Outdated
| - name: Capture trigger schemas | ||
| run: bun run scripts/capture-trigger-schemas.ts --from-existing |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if --from-existing is parsed anywhere in the capture script
rg -n 'from-existing' scripts/capture-trigger-schemas.tsRepository: elizaos-plugins/plugin-n8n-workflow
Length of output: 61
🏁 Script executed:
#!/bin/bash
# Look at the argument parsing logic around lines 306-310
sed -n '300,320p' scripts/capture-trigger-schemas.tsRepository: elizaos-plugins/plugin-n8n-workflow
Length of output: 931
🏁 Script executed:
#!/bin/bash
# Search for --from-existing across the entire codebase
rg -r 'from-existing' --type ts --type jsRepository: elizaos-plugins/plugin-n8n-workflow
Length of output: 475
🏁 Script executed:
#!/bin/bash
# Check the workflow file mentioned in the review
grep -n 'from-existing' .github/workflows/schema-update.ymlRepository: elizaos-plugins/plugin-n8n-workflow
Length of output: 153
🏁 Script executed:
#!/bin/bash
# Search for this flag in all workflow files
grep -rn 'from-existing' .github/workflows/Repository: elizaos-plugins/plugin-n8n-workflow
Length of output: 297
🏁 Script executed:
#!/bin/bash
# Check the full main() function to understand the behavior without this flag
sed -n '300,400p' scripts/capture-trigger-schemas.ts | head -60Repository: elizaos-plugins/plugin-n8n-workflow
Length of output: 2371
--from-existing flag is not recognized by capture-trigger-schemas.ts.
The script only parses --trigger=, --timeout=, --keep, and --create-only (lines 307-310). The --from-existing flag, which is passed in both .github/workflows/schema-update.yml (line 23) and .github/workflows/npm-deploy.yml (line 77), is silently ignored. The script will create and activate new workflows instead of only reading from existing ones, contrary to the intended behavior in CI.
🤖 Prompt for AI Agents
In @.github/workflows/schema-update.yml around lines 22 - 23, The CI flag
--from-existing is not parsed by capture-trigger-schemas.ts so it is ignored;
update the CLI parsing (where --trigger, --timeout, --keep, and --create-only
are handled) to accept a --from-existing boolean (e.g., set a fromExisting
variable in parseArgs or the existing argument-parsing block) and then propagate
that flag into the main flow (functions like captureTriggerSchemas /
createAndActivateWorkflows or whichever routines perform creation/activation) so
that when fromExisting is true the code only reads existing workflows and
explicitly skips any creation or activation steps.
| const KNOWN_CREDENTIALS: Record<string, string> = { | ||
| gmailOAuth2: 'hjSOdmEwxXzQpj3V', | ||
| googleCalendarOAuth2Api: 'yHjZgYhYPxTOFwdr', | ||
| googleDriveOAuth2Api: 'LoKYrCVJOpU8PhAM', | ||
| googleSheetsTriggerOAuth2Api: 'UcHkFKN0Khm7z1O3', | ||
| googleBusinessProfileOAuth2Api: 'lo6OS1caDfS84dgR', | ||
| githubOAuth2Api: 'T3JHHfCQmwbQgtEr', | ||
| linearOAuth2Api: 'jADHC6LxJ8ax7JMc', | ||
| }; |
There was a problem hiding this comment.
Hardcoded credential IDs committed to source control.
Gitleaks rightly flags these as secrets. Even though they are n8n-internal IDs (not raw API keys), they reference real credential entries on a specific instance and expose infrastructure details. Move them to environment variables or a .env file excluded from version control.
🛡️ Suggested approach
-const KNOWN_CREDENTIALS: Record<string, string> = {
- gmailOAuth2: 'hjSOdmEwxXzQpj3V',
- googleCalendarOAuth2Api: 'yHjZgYhYPxTOFwdr',
- ...
-};
+// Load from env: N8N_CRED_MAP='gmailOAuth2=abc123,googleCalendarOAuth2Api=def456'
+const KNOWN_CREDENTIALS: Record<string, string> = {};
+const credMapEnv = process.env.N8N_CRED_MAP ?? '';
+for (const pair of credMapEnv.split(',').filter(Boolean)) {
+ const [key, val] = pair.split('=');
+ if (key && val) KNOWN_CREDENTIALS[key.trim()] = val.trim();
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const KNOWN_CREDENTIALS: Record<string, string> = { | |
| gmailOAuth2: 'hjSOdmEwxXzQpj3V', | |
| googleCalendarOAuth2Api: 'yHjZgYhYPxTOFwdr', | |
| googleDriveOAuth2Api: 'LoKYrCVJOpU8PhAM', | |
| googleSheetsTriggerOAuth2Api: 'UcHkFKN0Khm7z1O3', | |
| googleBusinessProfileOAuth2Api: 'lo6OS1caDfS84dgR', | |
| githubOAuth2Api: 'T3JHHfCQmwbQgtEr', | |
| linearOAuth2Api: 'jADHC6LxJ8ax7JMc', | |
| }; | |
| // Load from env: N8N_CRED_MAP='gmailOAuth2=abc123,googleCalendarOAuth2Api=def456' | |
| const KNOWN_CREDENTIALS: Record<string, string> = {}; | |
| const credMapEnv = process.env.N8N_CRED_MAP ?? ''; | |
| for (const pair of credMapEnv.split(',').filter(Boolean)) { | |
| const [key, val] = pair.split('='); | |
| if (key && val) KNOWN_CREDENTIALS[key.trim()] = val.trim(); | |
| } |
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 136-136: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 138-138: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 139-139: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 140-140: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 141-141: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 142-142: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In `@scripts/capture-trigger-schemas.ts` around lines 135 - 143, The
KNOWN_CREDENTIALS constant currently contains hardcoded credential IDs; remove
the hardcoded map and load these IDs from environment variables instead (either
a single JSON string env like KNOWN_CREDENTIALS_JSON that you JSON.parse and
validate, or individual env vars per key such as KNOWN_CREDENTIALS_gmailOAuth2,
_googleCalendarOAuth2Api, etc.). Update scripts/capture-trigger-schemas.ts to
read process.env for these values, validate required keys exist (throw or log a
clear error if missing), and fallback to an empty object only for non-critical
keys; ensure no secret values remain in source control and document the expected
env var names in the project README or .env.example.
| async function main() { | ||
| const args = process.argv.slice(2); | ||
| const filterTrigger = args.find((a) => a.startsWith('--trigger='))?.split('=')[1]; | ||
| const timeoutSec = parseInt(args.find((a) => a.startsWith('--timeout='))?.split('=')[1] ?? '30'); | ||
| const keepWorkflows = args.includes('--keep'); | ||
| const createOnly = args.includes('--create-only'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if --from-existing is used in CI workflows
rg -n 'from-existing' .github/workflows/ || echo "No matches found in workflows"Repository: elizaos-plugins/plugin-n8n-workflow
Length of output: 297
🏁 Script executed:
# Check the full argument parsing in the script
cat -n scripts/capture-trigger-schemas.ts | head -350 | tail -100Repository: elizaos-plugins/plugin-n8n-workflow
Length of output: 4453
🏁 Script executed:
# Also check if --from-existing is referenced anywhere else in the script
rg -n 'from' scripts/capture-trigger-schemas.tsRepository: elizaos-plugins/plugin-n8n-workflow
Length of output: 592
🏁 Script executed:
# Search for camelCase or snake_case variants
rg -n 'fromExisting|from_existing|FromExisting' scripts/capture-trigger-schemas.tsRepository: elizaos-plugins/plugin-n8n-workflow
Length of output: 61
🏁 Script executed:
# Check the full main function to see flow and understand what should happen with the flag
sed -n '305,450p' scripts/capture-trigger-schemas.ts | cat -nRepository: elizaos-plugins/plugin-n8n-workflow
Length of output: 6361
--from-existing flag is used in CI workflows but not parsed.
Both .github/workflows/schema-update.yml:23 and .github/workflows/npm-deploy.yml:77 invoke this script with --from-existing, but the argument parsing on lines 305-310 doesn't handle it. The script will always proceed to create, activate, and wait for trigger workflows (lines 84-132) instead of only capturing schemas from existing workflows as intended.
🤖 Prompt for AI Agents
In `@scripts/capture-trigger-schemas.ts` around lines 305 - 310, The script's
main() argument parsing misses the --from-existing flag, so add parsing for it
(e.g., const fromExisting = args.includes('--from-existing')) alongside existing
flags (filterTrigger, timeoutSec, keepWorkflows, createOnly) and then use that
boolean to short-circuit the create/activate/wait flow in the trigger capture
logic (the block that currently creates, activates, and waits for triggers
between lines 84-132), making the script only capture schemas from existing
workflows when fromExisting is true; update any downstream conditionals that
currently check createOnly/keepWorkflows to also respect fromExisting where
appropriate.
| // Create draft from the existing workflow | ||
| const draft: WorkflowDraft = { | ||
| workflow: fullWorkflow, | ||
| prompt: `Modify existing workflow: ${fullWorkflow.name}`, | ||
| userId, | ||
| createdAt: Date.now(), | ||
| }; | ||
| await runtime.setCache(cacheKey, draft); |
There was a problem hiding this comment.
Draft is missing originMessageId — same-turn auto-confirm guard won't apply.
createWorkflow.ts (line 316) checks existingDraft.originMessageId === message.id to prevent auto-confirm on the same turn. Drafts created here lack originMessageId, so if an agent chains MODIFY_EXISTING_N8N_WORKFLOW → CREATE_N8N_WORKFLOW in one turn, the guard is bypassed and the draft could be auto-confirmed.
Proposed fix
const draft: WorkflowDraft = {
workflow: fullWorkflow,
prompt: `Modify existing workflow: ${fullWorkflow.name}`,
userId,
createdAt: Date.now(),
+ originMessageId: message.id,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Create draft from the existing workflow | |
| const draft: WorkflowDraft = { | |
| workflow: fullWorkflow, | |
| prompt: `Modify existing workflow: ${fullWorkflow.name}`, | |
| userId, | |
| createdAt: Date.now(), | |
| }; | |
| await runtime.setCache(cacheKey, draft); | |
| // Create draft from the existing workflow | |
| const draft: WorkflowDraft = { | |
| workflow: fullWorkflow, | |
| prompt: `Modify existing workflow: ${fullWorkflow.name}`, | |
| userId, | |
| createdAt: Date.now(), | |
| originMessageId: message.id, | |
| }; | |
| await runtime.setCache(cacheKey, draft); |
🤖 Prompt for AI Agents
In `@src/actions/modifyExistingWorkflow.ts` around lines 198 - 205, The draft
created in modifyExistingWorkflow.ts is missing the
WorkflowDraft.originMessageId field so the same-turn auto-confirm guard (which
checks existingDraft.originMessageId === message.id in createWorkflow.ts) can be
bypassed; fix by adding originMessageId: message.id (or the appropriate incoming
message identifier variable available in this scope) to the draft object before
calling runtime.setCache(cacheKey, draft) so the guard will correctly detect
same-turn drafts.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
PR Review SummaryI've completed a comprehensive review of this PR. Overall, the implementation is solid with good security practices. I've identified several issues ranging from critical security concerns to code quality improvements. Critical Issues 🔴
High Priority Issues 🟡
Code Quality Issues 🔵
See inline comments for specific fixes and recommendations. |
| googleBusinessProfileOAuth2Api: 'lo6OS1caDfS84dgR', | ||
| githubOAuth2Api: 'T3JHHfCQmwbQgtEr', | ||
| linearOAuth2Api: 'jADHC6LxJ8ax7JMc', | ||
| }; |
There was a problem hiding this comment.
🔴 CRITICAL SECURITY ISSUE: Hardcoded Credential IDs
These hardcoded credential IDs should never be committed to source control as they could provide unauthorized access to your n8n instance.
Recommended fixes:
- Move these to environment variables or a config file excluded from git
- Add
*credentials*.jsonto.gitignore - Document the credential setup process in README
| }; | |
| // Load from environment or secure config | |
| const KNOWN_CREDENTIALS: Record<string, string> = process.env.N8N_CREDENTIALS | |
| ? JSON.parse(process.env.N8N_CREDENTIALS) | |
| : {}; |
Consider rotating these credentials since they're now public in the PR.
| const refs: ExpressionRef[] = []; | ||
|
|
||
| // Match every $json.field reference, even inside compound expressions like {{ $json.a || $json.b }} | ||
| const simplePattern = /\$json\.([a-zA-Z0-9_.\[\]'"-]{1,200})/g; |
There was a problem hiding this comment.
🔴 CRITICAL: ReDoS (Regular Expression Denial of Service) Vulnerability
The regex patterns use unbounded quantifiers {1,200} with complex character classes, which can cause catastrophic backtracking on malicious input.
Attack scenario:
const malicious = "$json." + "a".repeat(1000) + "!";
// Could cause regex engine to hangRecommended fix:
| const simplePattern = /\$json\.([a-zA-Z0-9_.\[\]'"-]{1,200})/g; | |
| // Use atomic groups or possessive quantifiers, limit complexity | |
| const simplePattern = /\$json\.([a-zA-Z0-9_]+(?:\.[a-zA-Z0-9_]+|\[\d+\])*)/g; | |
| const namedNodePattern = /\$\(['"]([^'"]{1,100})['"]\)\.item\.json\.([a-zA-Z0-9_]+(?:\.[a-zA-Z0-9_]+|\[\d+\])*)/g; |
Additionally, add input length validation before regex matching:
if (str.length > 10000) return []; // Reject suspiciously long inputs
|
|
||
| // Medium/High confidence — load the workflow | ||
| const workflowId = matchResult.matchedWorkflowId; | ||
| const fullWorkflow = await service.getWorkflow(workflowId); |
There was a problem hiding this comment.
🟡 SECURITY: Missing Input Validation on Workflow ID
The workflow ID from matchedWorkflowId is passed directly to the API without validation. While n8n likely validates this, defense-in-depth suggests validating format here.
Potential issues:
- Path traversal if IDs can contain
../ - Injection if IDs can contain special characters
Recommended fix:
| const fullWorkflow = await service.getWorkflow(workflowId); | |
| const workflowId = matchResult.matchedWorkflowId; | |
| // Validate workflow ID format (assuming n8n uses alphanumeric IDs) | |
| if (!/^[a-zA-Z0-9-]+$/.test(workflowId)) { | |
| logger.error({ src: 'plugin:n8n-workflow:action:modify-existing' }, | |
| `Invalid workflow ID format: ${workflowId}`); | |
| if (callback) { | |
| await callback({ text: 'Invalid workflow identifier', success: false }); | |
| } | |
| return { success: false }; | |
| } | |
| const fullWorkflow = await service.getWorkflow(workflowId); |
|
|
||
| // Check for existing draft — if one exists, user should use CREATE_N8N_WORKFLOW | ||
| const existingDraft = await runtime.getCache<WorkflowDraft>(cacheKey); | ||
| if (existingDraft && Date.now() - existingDraft.createdAt < DRAFT_TTL_MS) { |
There was a problem hiding this comment.
🟡 BUG: Race Condition in TTL Check
There's a TOCTOU (Time-of-check Time-of-use) race condition. If the draft expires between the check and the response, the user gets inconsistent behavior.
Issue:
if (existingDraft && Date.now() - existingDraft.createdAt < DRAFT_TTL_MS) {
// Draft could expire here before callback executes
await callback({ text: '...' });
}Recommended fix:
| if (existingDraft && Date.now() - existingDraft.createdAt < DRAFT_TTL_MS) { | |
| const existingDraft = await runtime.getCache<WorkflowDraft>(cacheKey); | |
| const draftAge = existingDraft ? Date.now() - existingDraft.createdAt : null; | |
| if (existingDraft && draftAge !== null && draftAge < DRAFT_TTL_MS) { | |
| const remainingMinutes = Math.floor((DRAFT_TTL_MS - draftAge) / 60000); | |
| if (callback) { | |
| await callback({ | |
| text: `You already have a workflow draft in progress (expires in ${remainingMinutes}min). ` + | |
| 'Please confirm, modify, or cancel that draft first before loading another workflow.', | |
| success: false, | |
| }); | |
| } | |
| return { success: false }; | |
| } |
| const value = obj[key]; | ||
|
|
||
| if (typeof value === 'string' && value.includes(original)) { | ||
| obj[key] = value.replaceAll(original, replacement); |
There was a problem hiding this comment.
🟡 BUG: Unsafe String Replacement Could Cause Unintended Changes
Using replaceAll() with plain strings can cause unintended replacements if the original expression appears elsewhere in the workflow.
Example issue:
// If correcting "$json.name" → "$json.fullName"
// This would incorrectly replace:
"Please enter your name" → "Please enter your fullName"Recommended fix:
| obj[key] = value.replaceAll(original, replacement); | |
| if (typeof value === 'string' && value.includes(original)) { | |
| // Only replace if it's actually the expression, not just substring match | |
| // Use a more precise regex-based replacement | |
| const escapedOriginal = original.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | |
| const regex = new RegExp(escapedOriginal, 'g'); | |
| obj[key] = value.replace(regex, replacement); | |
| } |
Better yet, track the exact parameter path during validation and only replace in that specific location rather than doing global replacements.
| return false; | ||
| } | ||
|
|
||
| const prop = properties[segment] as SchemaContent | undefined; |
There was a problem hiding this comment.
🔵 Code Quality: Missing Type Safety
The type assertion as SchemaContent bypasses TypeScript's type checking. This could lead to runtime errors if the schema structure is malformed.
Recommended fix:
| const prop = properties[segment] as SchemaContent | undefined; | |
| const prop = properties[segment]; | |
| // Type guard instead of assertion | |
| if (!prop || typeof prop !== 'object' || !('type' in prop)) { | |
| return false; | |
| } | |
| const propSchema = prop as SchemaContent; | |
| if (!propSchema) { | |
| return false; | |
| } |
|
|
||
| function getSourceSchema(sourceName: string) { | ||
| if (schemaCache.has(sourceName)) { | ||
| return schemaCache.get(sourceName)!; |
There was a problem hiding this comment.
🔵 Code Quality: Potential Null Dereference
If schemaCache.get(sourceName) returns undefined, the ! assertion will cause the function to return null without proper type safety.
Recommended fix:
| return schemaCache.get(sourceName)!; | |
| function getSourceSchema(sourceName: string) { | |
| if (schemaCache.has(sourceName)) { | |
| return schemaCache.get(sourceName) ?? null; | |
| } |
This makes the null handling explicit and type-safe.
| /** | ||
| * Parses "from.value[0].address" or "headers['content-type']" into path segments. | ||
| */ | ||
| function parseFieldPath(field: string): string[] { |
There was a problem hiding this comment.
🔵 Code Quality: Complex Parsing Logic Without Bounds
The manual character-by-character parsing could be vulnerable to edge cases and doesn't have clear bounds checking.
Suggestions:
- Add a maximum path length check at the start
- Consider using a battle-tested parser library for complex paths
- Add unit tests for malformed inputs like:
- Unclosed brackets:
field[unclosed - Nested quotes:
field['nested\\'quote'] - Very deep nesting:
a.b.c.d.e.f.g...(200+ levels)
- Unclosed brackets:
function parseFieldPath(field: string): string[] {
if (field.length > 500) {
throw new Error('Field path exceeds maximum length');
}
// ... existing parsing logic
}
| }; | ||
| } | ||
|
|
||
| function diffNodeParams( |
There was a problem hiding this comment.
🔵 Code Quality: Inconsistent Parameter Comparison
Using JSON.stringify for object comparison has performance implications and can give false positives/negatives due to property ordering.
Issues:
{a:1, b:2}vs{b:2, a:1}would be considered different- Performance overhead for large parameter objects
- Doesn't handle
undefinedvalues correctly
Recommended approach:
function deepEqual(a: unknown, b: unknown): boolean {
if (a === b) return true;
if (typeof a !== typeof b) return false;
if (typeof a !== 'object' || a === null || b === null) return false;
const keysA = Object.keys(a);
const keysB = Object.keys(b as object);
if (keysA.length !== keysB.length) return false;
return keysA.every(key => deepEqual((a as any)[key], (b as any)[key]));
}Or use a library like lodash.isEqual for this.
|
|
||
| const workflow = await generateWorkflow( | ||
| let workflow = await generateWorkflow( | ||
| this.runtime, |
There was a problem hiding this comment.
🔵 Code Quality: Unhandled Promise Rejection
The generateWorkflow function call isn't wrapped in try-catch. If it throws, the error might not be properly logged or handled according to the service's error handling pattern.
While there is an outer try-catch at line 122, it's better to have explicit error handling for critical operations to provide better context.
Suggested improvement:
let workflow;
try {
workflow = await generateWorkflow(
this.runtime,
prompt,
relevantNodes.map((r) => r.node)
);
} catch (error) {
logger.error(
{ src: 'plugin:n8n-workflow:service:main' },
`Workflow generation failed: ${error instanceof Error ? error.message : String(error)}`
);
throw error; // Re-throw after logging with context
}
Additional ObservationsPositive Aspects ✅
Recommendations for Future
Test Coverage NeededPlease ensure tests cover:
|
1 participant
Summary
$json.fieldexpressions against upstream node output schemas, with type-aware auto-correction$jsonrefs inside||, ternary, and concatenation expressionssimple: trueon triggers that support it (Gmail, Notion, etc.) for predictable outputKey changes
Schema validation & correction
validateOutputReferences()checks every$json.fieldexpression against the source node's output schema$json.fieldindividually (not just{{ $json.field }}) — catches{{ $json.a || $json.b }}snippet (string)vspayload (object)) so LLM picks the right fieldnormalizeTriggerSimpleParam()forcessimple: trueon triggers that support it — ensures our captured schema matches runtime outputloadTriggerOutputSchema()returns null whensimple: false(raw mode output differs from captured schema)Trigger schema infrastructure
scripts/capture-trigger-schemas.ts— capture trigger output schemas from n8n executionsscripts/crawl-trigger-schemas.ts— discover all triggers and their configurations.github/workflows/schema-update.yml— weekly cron: regenerate schemas, compare with latest npm release, auto-bump version if changedDraft lifecycle
intentandmodificationaction parameters — multi-step agents can no longer bypassclassifyDraftIntent()originMessageIdtoWorkflowDraft— if the action is called again with the same message that created the draft, re-show preview instead of classifyingNew files
src/utils/outputSchema.ts— schema loading, field path extraction, expression parsing, type-aware field pathssrc/prompts/fieldCorrection.ts— type-aware LLM correction promptsrc/actions/modifyExistingWorkflow.ts— load existing deployed workflows for editingscripts/capture-trigger-schemas.ts,scripts/crawl-trigger-schemas.ts,scripts/create-credentials.ts.github/workflows/schema-update.ymlModified files
src/actions/createWorkflow.ts— removed explicit intent, added originMessageId guardsrc/utils/workflow.ts—validateOutputReferences(),normalizeTriggerSimpleParam(),isTriggerNode()helpersrc/utils/generation.ts—correctFieldReferences(), removed duplicate intent logsrc/services/n8n-workflow-service.ts— wired normalizeTriggerSimpleParam + validateOutputReferences.github/workflows/npm-deploy.yml— usescrawl+ trigger capture stepsTest plan
{{ $json.textHtml || $json.textPlain }}detected and correctedSummary by CodeRabbit
New Features
Bug Fixes
Tests