fix: prevent multi-step confabulation + ensure AI nodes in results#19
fix: prevent multi-step confabulation + ensure AI nodes in results#19
Conversation
…rch results
Bug 1: Callbacks now carry data: { awaitingUserInput: true } so the cloud
multi-step loop breaks correctly after preview/clarification/auth-required
states. Removes the originMessageId guard (dead code once callbacks signal
correctly).
Bug 2: supplementAINodes() ensures OpenAI and AI Transform nodes appear in
search results when AI-intent keywords are detected, even when service
keywords (gmail, trigger) dominate the scores.
Also patches modifyExistingWorkflow callback (same bug class as Bug 1).
WalkthroughRemoves Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Handler as Action Handler
participant Generator as Workflow Generator
participant Search as Node Search Service
participant Cache as Draft/Deletion Cache
participant Callback as Callback Handler
User->>Handler: Request create/modify workflow (with keywords)
Handler->>Generator: generateAndPreview(...)
Generator->>Search: searchNodes(keywords)
Search->>Search: supplementAINodes(keywords)
Search-->>Generator: combinedNodes
Generator->>Cache: persist draft (DRAFT_TTL_MS)
Generator->>Callback: callback(text, data: { awaitingUserInput: true })
Callback-->>User: Prompt for confirmation/input
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/services/n8n-workflow-service.ts (1)
151-189: Consider hoistingAI_INTENT_KEYWORDSto a module-level constant.The
Setis re-created on every invocation. Since the values are static, moving it outside the method avoids redundant allocations and makes the keyword list easier to locate and maintain.+const AI_INTENT_KEYWORDS = new Set([ + 'summarize', 'summary', 'translate', 'translation', 'classify', + 'categorize', 'extract', 'analyze', 'analysis', 'sentiment', + 'rewrite', 'detect', 'ai', 'llm', 'gpt', 'openai', +]); + // Inside the class: - private supplementAINodes(...) { - const AI_INTENT_KEYWORDS = new Set([...]); + private supplementAINodes( + mainResults: NodeSearchResult[], + keywords: string[] + ): NodeSearchResult[] { const hasAIIntent = keywords.some((kw) => AI_INTENT_KEYWORDS.has(kw.toLowerCase()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/n8n-workflow-service.ts` around lines 151 - 189, Hoist the AI_INTENT_KEYWORDS Set out of supplementAINodes to a module-level constant (e.g., define const AI_INTENT_KEYWORDS = new Set([...]) at the top of the file) and remove the local declaration inside the supplementAINodes method; keep the same lowercase keyword values and preserve the call that checks keywords.some((kw) => AI_INTENT_KEYWORDS.has(kw.toLowerCase())). Update any imports/exports only if you need to reuse the constant elsewhere, and ensure supplementAINodes now references the top-level AI_INTENT_KEYWORDS symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/n8n-workflow-service.ts`:
- Around line 174-175: The early return after computing hasAIIntent violates the
`curly` rule; update the conditional to use braces around the return (e.g.,
change the single-line `if (!hasAIIntent) return mainResults;` to a braced
block) in the function where `const hasAIIntent = keywords.some((kw) =>
AI_INTENT_KEYWORDS.has(kw.toLowerCase()));` is declared so the `if` uses `{ ...
}` around the `return mainResults;`.
---
Nitpick comments:
In `@src/services/n8n-workflow-service.ts`:
- Around line 151-189: Hoist the AI_INTENT_KEYWORDS Set out of supplementAINodes
to a module-level constant (e.g., define const AI_INTENT_KEYWORDS = new
Set([...]) at the top of the file) and remove the local declaration inside the
supplementAINodes method; keep the same lowercase keyword values and preserve
the call that checks keywords.some((kw) =>
AI_INTENT_KEYWORDS.has(kw.toLowerCase())). Update any imports/exports only if
you need to reuse the constant elsewhere, and ensure supplementAINodes now
references the top-level AI_INTENT_KEYWORDS symbol.
This comment was marked as spam.
This comment was marked as spam.
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}
Picks up two bug fixes from elizaos-plugins/plugin-n8n-workflow#19: - Multi-step loop confabulation: callbacks now carry data: { awaitingUserInput: true } so the cloud multi-step loop breaks correctly after preview/clarification/auth-required states. - AI nodes missing from search: supplementAINodes() ensures OpenAI and AI Transform nodes appear when AI-intent keywords are detected.
…y test - Remove over-broad keywords (extract, analyze, detect, classify, categorize) that would inject AI nodes for non-AI prompts like "extract attachments" - Add braces around early return to fix ESLint curly rule (CI failure) - Add comment clarifying show_preview test uses intentionally invalid intent - Remove historical comment about originMessageId
This comment was marked as outdated.
This comment was marked as outdated.
| mainResults: NodeSearchResult[], | ||
| keywords: string[] | ||
| ): NodeSearchResult[] { | ||
| const AI_INTENT_KEYWORDS = new Set([ |
There was a problem hiding this comment.
AI_INTENT_KEYWORDS is a Set allocated on every call to supplementAINodes. Because this method is invoked for both generateWorkflowDraft and modifyWorkflowDraft, it allocates and GC-collects a new Set on every user turn. Move it to module (or class-static) scope:
| const AI_INTENT_KEYWORDS = new Set([ | |
| private static readonly AI_INTENT_KEYWORDS = new Set([ | |
| 'summarize', | |
| 'summary', | |
| 'translate', | |
| 'translation', | |
| 'sentiment', | |
| 'rewrite', | |
| 'ai', | |
| 'llm', | |
| 'gpt', | |
| 'openai', | |
| ]); | |
| private supplementAINodes( | |
| mainResults: NodeSearchResult[], | |
| keywords: string[] | |
| ): NodeSearchResult[] { | |
| const AI_INTENT_KEYWORDS = N8nWorkflowService.AI_INTENT_KEYWORDS; |
| }); | ||
| if (callback) { | ||
| await callback({ text, success: true }); | ||
| await callback({ text, success: true, data: { awaitingUserInput: true } }); |
There was a problem hiding this comment.
Behaviour question: AUTH_REQUIRED now signals awaitingUserInput: true
With this change, the AUTH_REQUIRED callback (unresolved credentials) now carries data: { awaitingUserInput: true }. This is correct for the multi-step loop — the agent waits for the user to connect their services and re-confirm.
However, the draft is not deleted after the credentials check returns missing connections. If the user never connects the services, the draft remains cached for 30 minutes and a second "yes" attempt will re-run deployWorkflow again. This is probably the intended behaviour (retry without regenerating), but it is worth a comment explaining why the cache key is deliberately left intact here.
… shared constants, security hardening - Narrow broad catch to 404 only when updating workflows (prevents duplicate creation) - Add confirm-before-delete flow with cache-based pending deletion and 5min TTL - Extract DRAFT_TTL_MS to shared constant (was duplicated in 4 files) - Return HTTP 424 instead of 200 for missing credentials in REST routes - Remove N+1 API calls in workflowStatus provider (no per-workflow execution fetch) - Validate authUrl starts with https:// before embedding in Markdown - Fix overly broad trigger detection (use node.type only, not node.name) - Hoist AI_INTENT_KEYWORDS to module level (avoid re-creating Set per call) - Clarify draft retention after AUTH_REQUIRED (intentional retry without regeneration) - Update tests to match new confirm-before-delete and 424 status behavior
Review: fix/multi-step-loop-and-ai-nodesOverall AssessmentThe core fixes are sound — propagating Several issues need attention before merge, ranging from a potential runtime error to fragile UX in the new delete-confirmation flow. Issues🔴 High — Delete confirmation may never fireThe delete action's There's also no provider (analogous to Suggested mitigations:
🟡 Medium — Strict regex silently cancels on natural confirmations
🟡 Medium —
|
| const pending = await runtime.getCache<PendingDeletion>(cacheKey); | ||
| if (pending && Date.now() - pending.createdAt < DELETE_CONFIRM_TTL_MS) { | ||
| const userText = (message.content?.text || '').toLowerCase().trim(); | ||
| const isConfirm = /^(yes|confirm|ok|do it|go ahead|oui|y)$/i.test(userText); |
There was a problem hiding this comment.
The ^...$ anchors make this stricter than expected. Natural confirmation phrases like "yes please", "yes!", "Yes, do it.", or "yes go ahead" all fail the match and silently cancel the deletion instead.
Consider a contains-style check or an LLM intent classifier (consistent with classifyDraftIntent elsewhere):
| const isConfirm = /^(yes|confirm|ok|do it|go ahead|oui|y)$/i.test(userText); | |
| const isConfirm = /\b(yes|confirm|ok|do it|go ahead|oui)\b/i.test(userText) || userText === 'y'; |
Or, if strictness is intentional (to avoid false positives), at least tell the user why the deletion was cancelled so they can retype an explicit confirmation.
| return { success: true }; | ||
| } | ||
|
|
||
| // Not a confirm — cancel the pending deletion |
There was a problem hiding this comment.
When the input is not a confirmation keyword, the pending deletion is cancelled and the user is told "Deletion cancelled." — but they may have simply made a typo or asked a follow-up question. Consider distinguishing:
- Explicit cancel words (
no,cancel,stop,never mind) → cancel + inform - Unrecognised input → re-prompt ("Please reply 'yes' to confirm or 'no' to cancel"), keep the pending in cache
As written, any message that doesn't exactly match the confirm regex clears the cache, so the user has to restart the whole deletion flow.
| import { matchWorkflow } from '../utils/generation'; | ||
| import { buildConversationContext } from '../utils/context'; | ||
|
|
||
| const DELETE_CONFIRM_TTL_MS = 5 * 60 * 1000; |
There was a problem hiding this comment.
This PR centralises DRAFT_TTL_MS into src/utils/constants.ts, but DELETE_CONFIRM_TTL_MS remains a local magic number. For consistency and to make it easy to tune from one place, move it to the same constants file.
| const DELETE_CONFIRM_TTL_MS = 5 * 60 * 1000; | |
| import { DELETE_CONFIRM_TTL_MS } from '../utils/constants'; |
|
|
||
| try { | ||
| const userId = message.entityId; | ||
| const cacheKey = `workflow_delete_pending:${userId}`; |
There was a problem hiding this comment.
LLM routing gap: The confirmation loop relies on the ElizaOS orchestrator re-invoking DELETE_N8N_WORKFLOW when the user replies "yes". But the action's examples array only shows initial delete requests — there are no two-turn confirmation examples, and no provider (analogous to pendingDraftProvider) injects pending-deletion context into the system prompt.
Without this context, the LLM may route "yes" to a different action (e.g. CREATE_N8N_WORKFLOW if there's also a pending draft) or to no action at all, leaving the pending deletion in cache until the 5-minute TTL silently expires.
Consider adding:
- A
pendingDeletionProviderthat tells the LLM a deletion is awaiting confirmation. - A two-turn example in the
examplesarray to improve action routing.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/services/n8n-workflow-service.ts (1)
155-166: MoveAI_INTENT_KEYWORDSto a module-level constant.The
Setis reconstructed on every invocation ofsupplementAINodes. Hoisting it to module scope eliminates the redundant allocation.♻️ Proposed refactor
+const AI_INTENT_KEYWORDS = new Set([ + 'summarize', + 'summary', + 'translate', + 'translation', + 'sentiment', + 'rewrite', + 'ai', + 'llm', + 'gpt', + 'openai', +]); private supplementAINodes( mainResults: NodeSearchResult[], keywords: string[] ): NodeSearchResult[] { - const AI_INTENT_KEYWORDS = new Set([ - 'summarize', - 'summary', - 'translate', - 'translation', - 'sentiment', - 'rewrite', - 'ai', - 'llm', - 'gpt', - 'openai', - ]); - const hasAIIntent = keywords.some((kw) => AI_INTENT_KEYWORDS.has(kw.toLowerCase()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/n8n-workflow-service.ts` around lines 155 - 166, The Set constant AI_INTENT_KEYWORDS is being recreated inside supplementAINodes on every call; move its declaration to module scope (top-level) so it is constructed once and reused. Locate AI_INTENT_KEYWORDS and the function supplementAINodes in src/services/n8n-workflow-service.ts, hoist the new Set declaration out of the function to module-level scope, and update supplementAINodes to reference the hoisted AI_INTENT_KEYWORDS without changing its name or semantics.__tests__/integration/actions/createWorkflow.test.ts (1)
894-913: Consider scoping the negative assertions toawaitingUserInputonly.Lines 912 and 933 assert
expect(lastResult?.data).toBeUndefined(), which will break if any future change adds an unrelated field todataon successful deploy or cancel (e.g.,{ deployedId: 'wf-001' }). Scoping to the specific field tested by this suite is more resilient.♻️ Proposed refactor
- expect(lastResult?.data).toBeUndefined(); + expect((lastResult?.data as Record<string, unknown>)?.awaitingUserInput).toBeUndefined();Apply the same change at line 933.
Also applies to: 915-934
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/actions/createWorkflow.test.ts` around lines 894 - 913, The assertion currently checks the entire payload with expect(lastResult?.data).toBeUndefined(), which is brittle; change it to only assert the awaitingUserInput field is absent by replacing that assertion with expect(lastResult?.data?.awaitingUserInput).toBeUndefined() (and make the same change in the sibling test around the cancel case), keeping the rest of the test that invokes createWorkflowAction.handler, getLastCallbackResult(callback) and uses the same mock callback unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/services/n8n-workflow-service.ts`:
- Around line 169-171: The early-return now correctly uses braces around the if
statement (if (!hasAIIntent) { return mainResults; }), but the review contains a
leftover duplicate comment marker; remove the redundant
"[duplicate_comment]"/duplicate review note from the PR/comments and ensure the
code around hasAIIntent and mainResults in src/services/n8n-workflow-service.ts
remains unchanged and passes linting.
---
Nitpick comments:
In `@__tests__/integration/actions/createWorkflow.test.ts`:
- Around line 894-913: The assertion currently checks the entire payload with
expect(lastResult?.data).toBeUndefined(), which is brittle; change it to only
assert the awaitingUserInput field is absent by replacing that assertion with
expect(lastResult?.data?.awaitingUserInput).toBeUndefined() (and make the same
change in the sibling test around the cancel case), keeping the rest of the test
that invokes createWorkflowAction.handler, getLastCallbackResult(callback) and
uses the same mock callback unchanged.
In `@src/services/n8n-workflow-service.ts`:
- Around line 155-166: The Set constant AI_INTENT_KEYWORDS is being recreated
inside supplementAINodes on every call; move its declaration to module scope
(top-level) so it is constructed once and reused. Locate AI_INTENT_KEYWORDS and
the function supplementAINodes in src/services/n8n-workflow-service.ts, hoist
the new Set declaration out of the function to module-level scope, and update
supplementAINodes to reference the hoisted AI_INTENT_KEYWORDS without changing
its name or semantics.
| 'ai', | ||
| 'llm', | ||
| 'gpt', | ||
| 'openai', |
There was a problem hiding this comment.
The keyword set is narrow and vendor-specific. Common AI intent words that are missing: analyze, classify, extract, generate, chat, embedding, claude, anthropic, gemini, mistral. A user asking "summarize Slack messages with Claude" would match (summarize is present), but "analyze my emails with Anthropic" would not.
Also, several of these — gpt, openai — are vendor names rather than intents, which means the supplement only helps users who specifically name OpenAI, not users who want AI capability in general.
| return mainResults; | ||
| } | ||
|
|
||
| const aiResults = searchNodes(['openai', 'ai transform'], 5); |
There was a problem hiding this comment.
This hardcodes openai and ai transform as the supplemented nodes. Any user asking for Claude, Gemini, Mistral, or another AI provider gets OpenAI nodes injected instead of the provider they want. If the intent is to ensure some AI node is present, this may produce misleading workflow previews.
Consider either:
- Supplementing a broader set of LangChain/AI nodes (all
@n8n/n8n-nodes-langchain.*) - Or resolving the specific AI provider from the keywords before searching
|
|
||
| if (result?.status === 'needs_auth') { | ||
| missingConnections.push({ credType, authUrl: result.authUrl }); | ||
| const authUrl = result.authUrl.startsWith('https://') ? result.authUrl : undefined; |
There was a problem hiding this comment.
Good security improvement — prevents non-HTTPS redirect URLs from propagating to the UI. A couple of follow-up hardening suggestions:
-
The
CredentialProviderResulttype declaresauthUrl: string(not optional), but external providers could misbehave and returnundefined. Wrapping intypeof result.authUrl === 'string' &&costs nothing and prevents a potentialTypeErrorif a provider doesn't honour the contract. -
The scheme check doesn't prevent SSRF-style private-network URLs (
https://169.254.169.254,https://internal.corp). If the plugin is deployed server-side, consider documenting this scope limitation or adding a hostname allowlist.
| const hasTrigger = workflow.nodes.some( | ||
| (node) => isTriggerNode(node.type) || node.name.toLowerCase().includes('start') | ||
| ); | ||
| const hasTrigger = workflow.nodes.some((node) => isTriggerNode(node.type)); |
There was a problem hiding this comment.
Removing the node.name.toLowerCase().includes('start') heuristic is the right correctness call. Worth noting: the LLM occasionally emits a generic node with name: "Start" and a non-trigger type (e.g. n8n-nodes-base.noOp). The old code silently ignored this; the new code will produce a "Workflow has no trigger node" validation warning for those workflows.
This is a net improvement in strictness, but it may surface as a new class of generation warnings in staging. Keeping an eye on it during the staging test checklist is worthwhile.
| } | ||
|
|
||
| status += '\n'; | ||
| status += ` Nodes: ${workflow.nodes?.length || 0}\n\n`; |
There was a problem hiding this comment.
Removing the per-workflow execution fetch is a good performance improvement (was N+1 API calls per status request). However, the provider's name (n8n_workflow_status) and the removed test assertion (expect(result.text).toContain('success')) confirm that execution status is silently no longer available to downstream agents.
If any agent prompt or capability description mentions "last execution status", it should be updated to reflect this. Consider adding a note in the provider's JSDoc or a separate workflowExecutionsProvider for callers that need execution history.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
__tests__/integration/providers/providers.test.ts (2)
268-334:⚠️ Potential issue | 🔴 Critical
pendingDraftProvidertests broken by cache key / entityId mismatch — pipeline failure.The
createMockMessagecalls were updated to useentityId: 'custom-user-0000-0000-000000000001', but the four cache setup objects still use the key'workflow_draft:user-001'.pendingDraftProviderconstructscacheKey = \workflow_draft:${message.entityId}`and therefore looks up'workflow_draft:custom-user-0000-0000-000000000001'`, which is absent.Impact per test:
Test Line Result returns draft info when draft exists286 FAILS (CI-confirmed) returns empty for expired draft310–311 passes for wrong reason (key not found, not TTL expiry) includes node names in text332–333 FAILS scoped to user — no draft for other user354 passes for wrong reason 🐛 Proposed fix — update all four cache-key strings to match the new entityId
- 'workflow_draft:user-001': { + 'workflow_draft:custom-user-0000-0000-000000000001': { workflow: draftWorkflow, prompt: 'Send gmail to telegram', - userId: 'user-001', + userId: 'custom-user-0000-0000-000000000001', createdAt: Date.now(), },Apply the same replacement to the cache blocks in the
returns empty for expired drafttest (lines ~294-300), theincludes node names in texttest (lines ~316-322), and thescoped to user — no draft for other usertest (lines ~338-344).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/providers/providers.test.ts` around lines 268 - 334, The tests fail because createMockMessage now uses entityId 'custom-user-0000-0000-000000000001' but the cache entries still use the old key 'workflow_draft:user-001', so pendingDraftProvider.get (which builds cacheKey = `workflow_draft:${message.entityId}`) can't find the drafts; update each cache object used in the tests (the four occurrences in the tests including in the "returns draft info when draft exists", "returns empty for expired draft", "includes node names in text", and "scoped to user — no draft for other user" blocks) to use the matching key 'workflow_draft:custom-user-0000-0000-000000000001' so pendingDraftProvider.get locates the intended draft entries when calling createMockMessage with that entityId.
154-207:⚠️ Potential issue | 🟡 Minor
getWorkflowExecutionsmocks are dead code after the N+1 removal — stale tests.
workflowStatus.tsno longer callsgetWorkflowExecutions(N+1 queries removed). Both tests affected:
includes workflow status and execution info(line 154): thegetWorkflowExecutionsmock is never invoked; test name is now misleading.handles execution fetch error per workflow(line 188): test intent ("Should still return workflow info even if executions fail") no longer applies.Both tests still pass by coincidence because the workflow listing path is unchanged, but they are verifying non-existent behavior.
♻️ Suggested cleanup
For
includes workflow status and execution info:- getWorkflowExecutions: mock(() => - Promise.resolve([ - createExecution({ - status: 'success', - startedAt: '2025-01-15T10:30:00.000Z', - }), - ]) - ),For
handles execution fetch error per workflow, consider removing the entire test (it tests removed behavior) or renaming it to reflect what it actually verifies today (graceful handling oflistWorkflowsnot being a concern, since it doesn't throw in this path).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/providers/providers.test.ts` around lines 154 - 207, The tests reference getWorkflowExecutions which is no longer used by workflowStatusProvider.get; update tests accordingly by removing the dead getWorkflowExecutions mocks and either rename or delete the affected tests: for the "includes workflow status and execution info" test remove the execution-related setup and rename to reflect it only verifies listWorkflows output (e.g., "includes workflow status"), and for "handles execution fetch error per workflow" either delete the test entirely or change it to assert graceful behavior of workflowStatusProvider.get when listWorkflows returns data/errors; locate usages in the tests named 'includes workflow status and execution info' and 'handles execution fetch error per workflow' to make these edits.
🧹 Nitpick comments (5)
src/actions/deleteWorkflow.ts (2)
15-15: Consider movingDELETE_CONFIRM_TTL_MStosrc/utils/constants.tsfor consistency.The PR centralises
DRAFT_TTL_MSinconstants.ts; the newDELETE_CONFIRM_TTL_MSfollows the same TTL-constant pattern and is a natural companion there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/deleteWorkflow.ts` at line 15, Move the DELETE_CONFIRM_TTL_MS constant out of deleteWorkflow and into the shared constants module where DRAFT_TTL_MS lives (constants.ts), then replace the local definition in deleteWorkflow.ts with an import of DELETE_CONFIRM_TTL_MS from that constants module; ensure the exported name matches existing usages and update the import statements in deleteWorkflow.ts to reference the centralized constant.
113-113: Redundantiflag —userTextis already lowercased.
userTextis computed via.toLowerCase().trim()on the line above, so the/iflag on the confirmation regex is unnecessary.-const isConfirm = /^(yes|confirm|ok|do it|go ahead|oui|y)$/i.test(userText); +const isConfirm = /^(yes|confirm|ok|do it|go ahead|oui|y)$/.test(userText);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/deleteWorkflow.ts` at line 113, The regex used to set isConfirm redundantly includes the case-insensitive flag while userText is already set via .toLowerCase().trim(); update the pattern in deleteWorkflow.ts by removing the /i flag (change /^(yes|confirm|ok|do it|go ahead|oui|y)$/i to /^(yes|confirm|ok|do it|go ahead|oui|y)$/) so isConfirm uses the lowercase userText correctly; ensure this change targets the isConfirm assignment where userText is defined.__tests__/integration/actions/lifecycleActions.test.ts (1)
289-303: Test name "delete success" is misleading — it tests only the confirmation prompt, not actual deletion.The handler is called once (step 1 only), so
lastResultis the confirmation prompt callback (success: true). No deletion actually occurs in this test. Consider renaming todelete confirmation prompt returns success: true in callbackto reduce confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/actions/lifecycleActions.test.ts` around lines 289 - 303, The test named "delete success returns success: true in callback" is misleading because it only exercises the confirmation prompt (calling deleteWorkflowAction.handler once) and asserts the prompt callback result; rename the test to something explicit like "delete confirmation prompt returns success: true in callback" and update the test description string in the test block (the test(...) invocation) so it accurately references the confirmation prompt; keep the existing call to deleteWorkflowAction.handler, the createMockMessage with text 'Delete Stripe', createStateWithWorkflows, and assertions as-is.src/services/n8n-workflow-service.ts (1)
165-186:supplementAINodesimplementation is clean and correct.Dedup via
existingNamesSet prevents duplicates, and the method is appropriately scoped asprivate. The debug logging is helpful for diagnosing supplementation in production.One minor observation: the search terms
['openai', 'ai transform']are hardcoded. If additional AI nodes are added to the catalog later, they won't be picked up here. Consider co-locating these withAI_INTENT_KEYWORDSor extracting them to a constant for discoverability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/n8n-workflow-service.ts` around lines 165 - 186, The hardcoded search terms in supplementAINodes (the array passed to searchNodes(['openai', 'ai transform'], 5)) should be extracted into a named constant co-located with AI_INTENT_KEYWORDS (e.g., AI_NODE_SEARCH_TERMS) so future AI node names are discoverable and maintainable; replace the inline array in supplementAINodes with that constant and ensure the new constant is exported or defined near AI_INTENT_KEYWORDS so it’s easy to update when new AI nodes are added.src/routes/workflows.ts (1)
72-80: HTTP 424 for missing credentials is semantically correct.This is a breaking change for any API consumers that previously relied on a
200response withsuccess: falseandreason: 'missing_integrations'. If external clients consume this API, consider documenting the status code change in release notes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/workflows.ts` around lines 72 - 80, The handler currently returns HTTP 424 for missing credentials (res.status(424).json(...)) which breaks clients expecting a 200 with success:false; change the response back to 200 while preserving the payload shape (keep success:false, reason:'missing_integrations', missingIntegrations and warnings) so existing consumers continue to work, and add an inline comment near this block (in workflows.ts around the result.missingCredentials check) indicating the semantic vs. compatibility tradeoff and a TODO to document the status-code change in release notes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/integration/actions/lifecycleActions.test.ts`:
- Around line 191-216: The test for the two-step delete flow is missing an
assertion that the first call signals awaiting user input; after calling
deleteWorkflowAction.handler the test should assert that the returned payload
(result1) includes data.awaitingUserInput === true (and/or that the callback was
invoked with a message containing data.awaitingUserInput: true) so the
multi-step loop can continue; update the Step 1 assertions in the test that uses
deleteWorkflowAction.handler, createMockMessage, createStateWithWorkflows and
the callback, and keep the existing checks that service.deleteWorkflow was not
called.
In `@src/actions/deleteWorkflow.ts`:
- Around line 105-142: The pending-deletion entry is not removed when it exists
but is expired; update the logic around
runtime.getCache<PendingDeletion>(cacheKey) and the DELETE_CONFIRM_TTL_MS check
so that if pending exists but Date.now() - pending.createdAt >=
DELETE_CONFIRM_TTL_MS you call runtime.deleteCache(cacheKey) to clear the stale
entry before proceeding to the "start a new deletion flow" branch; preserve
existing behavior for the confirmed and non-confirmed branches (using
service.deleteWorkflow, callback, and logging) and ensure you reference the same
cacheKey/pending variables so no duplicate cache entries remain.
---
Outside diff comments:
In `@__tests__/integration/providers/providers.test.ts`:
- Around line 268-334: The tests fail because createMockMessage now uses
entityId 'custom-user-0000-0000-000000000001' but the cache entries still use
the old key 'workflow_draft:user-001', so pendingDraftProvider.get (which builds
cacheKey = `workflow_draft:${message.entityId}`) can't find the drafts; update
each cache object used in the tests (the four occurrences in the tests including
in the "returns draft info when draft exists", "returns empty for expired
draft", "includes node names in text", and "scoped to user — no draft for other
user" blocks) to use the matching key
'workflow_draft:custom-user-0000-0000-000000000001' so pendingDraftProvider.get
locates the intended draft entries when calling createMockMessage with that
entityId.
- Around line 154-207: The tests reference getWorkflowExecutions which is no
longer used by workflowStatusProvider.get; update tests accordingly by removing
the dead getWorkflowExecutions mocks and either rename or delete the affected
tests: for the "includes workflow status and execution info" test remove the
execution-related setup and rename to reflect it only verifies listWorkflows
output (e.g., "includes workflow status"), and for "handles execution fetch
error per workflow" either delete the test entirely or change it to assert
graceful behavior of workflowStatusProvider.get when listWorkflows returns
data/errors; locate usages in the tests named 'includes workflow status and
execution info' and 'handles execution fetch error per workflow' to make these
edits.
---
Duplicate comments:
In `@src/services/n8n-workflow-service.ts`:
- Around line 170-172: The curly-brace lint issue has already been fixed for the
early return around hasAIIntent; ensure the if block using hasAIIntent and
returning mainResults remains wrapped in braces (if (!hasAIIntent) { return
mainResults; }) and remove any stale duplicate review/comment markers; verify
the code paths in the function containing hasAIIntent and mainResults remain
logically unchanged after this change.
---
Nitpick comments:
In `@__tests__/integration/actions/lifecycleActions.test.ts`:
- Around line 289-303: The test named "delete success returns success: true in
callback" is misleading because it only exercises the confirmation prompt
(calling deleteWorkflowAction.handler once) and asserts the prompt callback
result; rename the test to something explicit like "delete confirmation prompt
returns success: true in callback" and update the test description string in the
test block (the test(...) invocation) so it accurately references the
confirmation prompt; keep the existing call to deleteWorkflowAction.handler, the
createMockMessage with text 'Delete Stripe', createStateWithWorkflows, and
assertions as-is.
In `@src/actions/deleteWorkflow.ts`:
- Line 15: Move the DELETE_CONFIRM_TTL_MS constant out of deleteWorkflow and
into the shared constants module where DRAFT_TTL_MS lives (constants.ts), then
replace the local definition in deleteWorkflow.ts with an import of
DELETE_CONFIRM_TTL_MS from that constants module; ensure the exported name
matches existing usages and update the import statements in deleteWorkflow.ts to
reference the centralized constant.
- Line 113: The regex used to set isConfirm redundantly includes the
case-insensitive flag while userText is already set via .toLowerCase().trim();
update the pattern in deleteWorkflow.ts by removing the /i flag (change
/^(yes|confirm|ok|do it|go ahead|oui|y)$/i to /^(yes|confirm|ok|do it|go
ahead|oui|y)$/) so isConfirm uses the lowercase userText correctly; ensure this
change targets the isConfirm assignment where userText is defined.
In `@src/routes/workflows.ts`:
- Around line 72-80: The handler currently returns HTTP 424 for missing
credentials (res.status(424).json(...)) which breaks clients expecting a 200
with success:false; change the response back to 200 while preserving the payload
shape (keep success:false, reason:'missing_integrations', missingIntegrations
and warnings) so existing consumers continue to work, and add an inline comment
near this block (in workflows.ts around the result.missingCredentials check)
indicating the semantic vs. compatibility tradeoff and a TODO to document the
status-code change in release notes.
In `@src/services/n8n-workflow-service.ts`:
- Around line 165-186: The hardcoded search terms in supplementAINodes (the
array passed to searchNodes(['openai', 'ai transform'], 5)) should be extracted
into a named constant co-located with AI_INTENT_KEYWORDS (e.g.,
AI_NODE_SEARCH_TERMS) so future AI node names are discoverable and maintainable;
replace the inline array in supplementAINodes with that constant and ensure the
new constant is exported or defined near AI_INTENT_KEYWORDS so it’s easy to
update when new AI nodes are added.
| test('deletes matched workflow after confirmation', async () => { | ||
| const { runtime, service } = createRuntimeWithMatchingWorkflow(); | ||
| const message = createMockMessage({ | ||
| content: { text: 'Delete the Stripe workflow' }, | ||
| }); | ||
| const callback = createMockCallback(); | ||
|
|
||
| const result = await deleteWorkflowAction.handler( | ||
| // Step 1: request deletion → gets confirmation prompt | ||
| const result1 = await deleteWorkflowAction.handler( | ||
| runtime, | ||
| message, | ||
| createMockMessage({ content: { text: 'Delete the Stripe workflow' } }), | ||
| createStateWithWorkflows(), | ||
| {}, | ||
| callback | ||
| ); | ||
| expect(result1.success).toBe(true); | ||
| expect(service.deleteWorkflow).not.toHaveBeenCalled(); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| // Step 2: confirm → actually deletes | ||
| const result2 = await deleteWorkflowAction.handler( | ||
| runtime, | ||
| createMockMessage({ content: { text: 'yes' } }), | ||
| createStateWithWorkflows(), | ||
| {}, | ||
| callback | ||
| ); | ||
| expect(result2.success).toBe(true); | ||
| expect(service.deleteWorkflow).toHaveBeenCalledWith('wf-001'); | ||
| }); |
There was a problem hiding this comment.
Step 1 of two-step delete is missing the core awaitingUserInput assertion.
The test verifies result1.success === true and service.deleteWorkflow not called, but the primary fix this PR ships (Bug 1 variant) is that the callback/return payload includes data: { awaitingUserInput: true }. That signal is what breaks the multi-step loop, and it's not asserted here.
💚 Suggested additions to step 1 assertions
expect(result1.success).toBe(true);
expect(service.deleteWorkflow).not.toHaveBeenCalled();
+ expect(result1.data).toEqual({ awaitingUserInput: true });
+ const promptCallback = (callback as any).mock.calls[0][0];
+ expect(promptCallback.data).toEqual({ awaitingUserInput: true });📝 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.
| test('deletes matched workflow after confirmation', async () => { | |
| const { runtime, service } = createRuntimeWithMatchingWorkflow(); | |
| const message = createMockMessage({ | |
| content: { text: 'Delete the Stripe workflow' }, | |
| }); | |
| const callback = createMockCallback(); | |
| const result = await deleteWorkflowAction.handler( | |
| // Step 1: request deletion → gets confirmation prompt | |
| const result1 = await deleteWorkflowAction.handler( | |
| runtime, | |
| message, | |
| createMockMessage({ content: { text: 'Delete the Stripe workflow' } }), | |
| createStateWithWorkflows(), | |
| {}, | |
| callback | |
| ); | |
| expect(result1.success).toBe(true); | |
| expect(service.deleteWorkflow).not.toHaveBeenCalled(); | |
| expect(result.success).toBe(true); | |
| // Step 2: confirm → actually deletes | |
| const result2 = await deleteWorkflowAction.handler( | |
| runtime, | |
| createMockMessage({ content: { text: 'yes' } }), | |
| createStateWithWorkflows(), | |
| {}, | |
| callback | |
| ); | |
| expect(result2.success).toBe(true); | |
| expect(service.deleteWorkflow).toHaveBeenCalledWith('wf-001'); | |
| }); | |
| test('deletes matched workflow after confirmation', async () => { | |
| const { runtime, service } = createRuntimeWithMatchingWorkflow(); | |
| const callback = createMockCallback(); | |
| // Step 1: request deletion → gets confirmation prompt | |
| const result1 = await deleteWorkflowAction.handler( | |
| runtime, | |
| createMockMessage({ content: { text: 'Delete the Stripe workflow' } }), | |
| createStateWithWorkflows(), | |
| {}, | |
| callback | |
| ); | |
| expect(result1.success).toBe(true); | |
| expect(service.deleteWorkflow).not.toHaveBeenCalled(); | |
| expect(result1.data).toEqual({ awaitingUserInput: true }); | |
| const promptCallback = (callback as any).mock.calls[0][0]; | |
| expect(promptCallback.data).toEqual({ awaitingUserInput: true }); | |
| // Step 2: confirm → actually deletes | |
| const result2 = await deleteWorkflowAction.handler( | |
| runtime, | |
| createMockMessage({ content: { text: 'yes' } }), | |
| createStateWithWorkflows(), | |
| {}, | |
| callback | |
| ); | |
| expect(result2.success).toBe(true); | |
| expect(service.deleteWorkflow).toHaveBeenCalledWith('wf-001'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@__tests__/integration/actions/lifecycleActions.test.ts` around lines 191 -
216, The test for the two-step delete flow is missing an assertion that the
first call signals awaiting user input; after calling
deleteWorkflowAction.handler the test should assert that the returned payload
(result1) includes data.awaitingUserInput === true (and/or that the callback was
invoked with a message containing data.awaitingUserInput: true) so the
multi-step loop can continue; update the Step 1 assertions in the test that uses
deleteWorkflowAction.handler, createMockMessage, createStateWithWorkflows and
the callback, and keep the existing checks that service.deleteWorkflow was not
called.
| try { | ||
| const userId = message.entityId; | ||
| const cacheKey = `workflow_delete_pending:${userId}`; | ||
|
|
||
| // Check for pending confirmation | ||
| const pending = await runtime.getCache<PendingDeletion>(cacheKey); | ||
| if (pending && Date.now() - pending.createdAt < DELETE_CONFIRM_TTL_MS) { | ||
| const userText = (message.content?.text || '').toLowerCase().trim(); | ||
| const isConfirm = /^(yes|confirm|ok|do it|go ahead|oui|y)$/i.test(userText); | ||
|
|
||
| if (isConfirm) { | ||
| await service.deleteWorkflow(pending.workflowId); | ||
| await runtime.deleteCache(cacheKey); | ||
|
|
||
| logger.info( | ||
| { src: 'plugin:n8n-workflow:action:delete' }, | ||
| `Deleted workflow ${pending.workflowId} after confirmation` | ||
| ); | ||
|
|
||
| if (callback) { | ||
| await callback({ | ||
| text: `Workflow "${pending.workflowName}" deleted permanently.`, | ||
| success: true, | ||
| }); | ||
| } | ||
| return { success: true }; | ||
| } | ||
|
|
||
| // Not a confirm — cancel the pending deletion | ||
| await runtime.deleteCache(cacheKey); | ||
| if (callback) { | ||
| await callback({ | ||
| text: 'Deletion cancelled.', | ||
| success: true, | ||
| }); | ||
| } | ||
| return { success: true }; | ||
| } |
There was a problem hiding this comment.
Stale expired pending-deletion cache entry not cleaned up before falling through.
When pending exists but the TTL has expired (Date.now() - pending.createdAt >= DELETE_CONFIRM_TTL_MS), the condition on line 111 is false and the code falls into the "No pending — start a new deletion flow" branch without deleting the stale cache entry. The entry self-heals the next time a new deletion request stores a fresh entry, but in the interim a "yes" message lands in matchWorkflow (which tries to match it as a workflow name) and the user gets a confusing "Could not identify which workflow to delete" response.
🛡️ Proposed fix — expire stale cache entry explicitly
const pending = await runtime.getCache<PendingDeletion>(cacheKey);
if (pending && Date.now() - pending.createdAt < DELETE_CONFIRM_TTL_MS) {
// ... handle confirmation / cancellation ...
}
+// If pending existed but TTL expired, clean it up before starting fresh
+if (pending) {
+ await runtime.deleteCache(cacheKey);
+}
// No pending — start a new deletion flow
const workflows = await service.listWorkflows(userId);📝 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.
| try { | |
| const userId = message.entityId; | |
| const cacheKey = `workflow_delete_pending:${userId}`; | |
| // Check for pending confirmation | |
| const pending = await runtime.getCache<PendingDeletion>(cacheKey); | |
| if (pending && Date.now() - pending.createdAt < DELETE_CONFIRM_TTL_MS) { | |
| const userText = (message.content?.text || '').toLowerCase().trim(); | |
| const isConfirm = /^(yes|confirm|ok|do it|go ahead|oui|y)$/i.test(userText); | |
| if (isConfirm) { | |
| await service.deleteWorkflow(pending.workflowId); | |
| await runtime.deleteCache(cacheKey); | |
| logger.info( | |
| { src: 'plugin:n8n-workflow:action:delete' }, | |
| `Deleted workflow ${pending.workflowId} after confirmation` | |
| ); | |
| if (callback) { | |
| await callback({ | |
| text: `Workflow "${pending.workflowName}" deleted permanently.`, | |
| success: true, | |
| }); | |
| } | |
| return { success: true }; | |
| } | |
| // Not a confirm — cancel the pending deletion | |
| await runtime.deleteCache(cacheKey); | |
| if (callback) { | |
| await callback({ | |
| text: 'Deletion cancelled.', | |
| success: true, | |
| }); | |
| } | |
| return { success: true }; | |
| } | |
| try { | |
| const userId = message.entityId; | |
| const cacheKey = `workflow_delete_pending:${userId}`; | |
| // Check for pending confirmation | |
| const pending = await runtime.getCache<PendingDeletion>(cacheKey); | |
| if (pending && Date.now() - pending.createdAt < DELETE_CONFIRM_TTL_MS) { | |
| const userText = (message.content?.text || '').toLowerCase().trim(); | |
| const isConfirm = /^(yes|confirm|ok|do it|go ahead|oui|y)$/i.test(userText); | |
| if (isConfirm) { | |
| await service.deleteWorkflow(pending.workflowId); | |
| await runtime.deleteCache(cacheKey); | |
| logger.info( | |
| { src: 'plugin:n8n-workflow:action:delete' }, | |
| `Deleted workflow ${pending.workflowId} after confirmation` | |
| ); | |
| if (callback) { | |
| await callback({ | |
| text: `Workflow "${pending.workflowName}" deleted permanently.`, | |
| success: true, | |
| }); | |
| } | |
| return { success: true }; | |
| } | |
| // Not a confirm — cancel the pending deletion | |
| await runtime.deleteCache(cacheKey); | |
| if (callback) { | |
| await callback({ | |
| text: 'Deletion cancelled.', | |
| success: true, | |
| }); | |
| } | |
| return { success: true }; | |
| } | |
| // If pending existed but TTL expired, clean it up before starting fresh | |
| if (pending) { | |
| await runtime.deleteCache(cacheKey); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/actions/deleteWorkflow.ts` around lines 105 - 142, The pending-deletion
entry is not removed when it exists but is expired; update the logic around
runtime.getCache<PendingDeletion>(cacheKey) and the DELETE_CONFIRM_TTL_MS check
so that if pending exists but Date.now() - pending.createdAt >=
DELETE_CONFIRM_TTL_MS you call runtime.deleteCache(cacheKey) to clear the stale
entry before proceeding to the "start a new deletion flow" branch; preserve
existing behavior for the confirmed and non-confirmed branches (using
service.deleteWorkflow, callback, and logging) and ensure you reference the same
cacheKey/pending variables so no duplicate cache entries remain.
1 participant
Summary
data: { awaitingUserInput: true }so the cloud multi-step loop breaks correctly after preview/clarification/auth-required states. Removes the deadoriginMessageIdguard.supplementAINodes()ensures OpenAI and AI Transform nodes appear in search results when AI-intent keywords are detected, even when service keywords (gmail, trigger) dominate the top 15 scores.awaitingUserInputsignal patched in the modify-existing action callback.awaitingUserInputtests, 2 new AI node scoring tests, removed obsoleteoriginMessageIdtests.Changed files
src/actions/createWorkflow.tsawaitingUserInput,originMessageIdguard removedsrc/actions/modifyExistingWorkflow.tsawaitingUserInputsrc/services/n8n-workflow-service.tssupplementAINodes()+ 17 AI-intent keywordssrc/types/index.tsoriginMessageIdremoved fromWorkflowDraft__tests__/integration/actions/createWorkflow.test.ts__tests__/unit/catalog.test.tspackage.jsonTest plan
bun test)bun run build)Summary by CodeRabbit
New Features
Improvements
Tests
Chores