Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 72 additions & 0 deletions packages/server/src/utils/buildAgentflow.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import { findChatMessageActionToClear } from './buildAgentflow'
import { ChatMessage } from '../database/entities/ChatMessage'

const makeMessage = (id: string, action: object | null): ChatMessage => {
const message = new ChatMessage()
message.id = id
message.action = action ? JSON.stringify(action) : null
return message
}

describe('findChatMessageActionToClear', () => {
it('returns the message whose action.data.nodeId matches the resumed startNodeId', () => {
const messages = [
makeMessage('msg-3', { data: { nodeId: 'humanInput_2' } }),
makeMessage('msg-2', { data: { nodeId: 'humanInput_1' } }),
makeMessage('msg-1', null)
]

const result = findChatMessageActionToClear(messages, 'humanInput_1')

expect(result?.id).toBe('msg-2')
})

it('does not clear a different pending Human-In-The-Loop node just because it is newer (#4787)', () => {
// Two HIL nodes are pending at the same time. Resuming humanInput_1 must not touch
// the chat message that belongs to humanInput_2, even though it was created more recently.
const newerUnrelatedAction = makeMessage('msg-newer', { data: { nodeId: 'humanInput_2' } })
const olderTargetAction = makeMessage('msg-older', { data: { nodeId: 'humanInput_1' } })
const messages = [newerUnrelatedAction, olderTargetAction]

const result = findChatMessageActionToClear(messages, 'humanInput_1')

expect(result?.id).toBe('msg-older')
expect(result?.id).not.toBe(newerUnrelatedAction.id)
})

it('returns undefined when no message matches the startNodeId', () => {
const messages = [makeMessage('msg-1', { data: { nodeId: 'humanInput_2' } })]

const result = findChatMessageActionToClear(messages, 'humanInput_1')

expect(result).toBeUndefined()
})

it('returns undefined when startNodeId is not provided', () => {
const messages = [makeMessage('msg-1', { data: { nodeId: 'humanInput_1' } })]

const result = findChatMessageActionToClear(messages, undefined)

expect(result).toBeUndefined()
})

it('skips messages with malformed action JSON instead of throwing', () => {
const malformed = new ChatMessage()
malformed.id = 'msg-malformed'
malformed.action = '{not valid json'

const valid = makeMessage('msg-valid', { data: { nodeId: 'humanInput_1' } })

const result = findChatMessageActionToClear([malformed, valid], 'humanInput_1')

expect(result?.id).toBe('msg-valid')
})

it('skips messages with no action set', () => {
const messages = [makeMessage('msg-1', null), makeMessage('msg-2', { data: { nodeId: 'humanInput_1' } })]

const result = findChatMessageActionToClear(messages, 'humanInput_1')

expect(result?.id).toBe('msg-2')
})
})
41 changes: 27 additions & 14 deletions packages/server/src/utils/buildAgentflow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2313,7 +2313,8 @@ export const executeAgentFlow = async ({
}
}

// Find the previous chat message with the same session/chat id and remove the action
// Clear the action on the specific message tied to the resumed node (not just the newest
// pending action) - a flow can have multiple HIL nodes pending at once. See #4787.
if (humanInput && Object.keys(humanInput).length) {
let query = await appDataSource
.getRepository(ChatMessage)
Expand All @@ -2323,19 +2324,13 @@ export const executeAgentFlow = async ({
.orderBy('chat_message.createdDate', 'DESC')
.getMany()

for (const result of query) {
if (result.action) {
try {
const newChatMessage = new ChatMessage()
Object.assign(newChatMessage, result)
newChatMessage.action = null
const cm = await appDataSource.getRepository(ChatMessage).create(newChatMessage)
await appDataSource.getRepository(ChatMessage).save(cm)
break
} catch (e) {
// error converting action to JSON
}
}
const messageToClear = findChatMessageActionToClear(query, humanInput.startNodeId)
if (messageToClear) {
const newChatMessage = new ChatMessage()
Object.assign(newChatMessage, messageToClear)
newChatMessage.action = null
const cm = await appDataSource.getRepository(ChatMessage).create(newChatMessage)
await appDataSource.getRepository(ChatMessage).save(cm)
}
}

Expand Down Expand Up @@ -2469,3 +2464,21 @@ export const executeAgentFlow = async ({
export const isObjectNotEmpty = (obj: any): boolean => {
return obj && Object.keys(obj).length > 0 && obj.constructor === Object
}

/**
* Finds the chat message whose pending action belongs to the given (resumed) HIL node.
* Matches by nodeId instead of recency, since multiple HIL nodes can be pending at once. See #4787.
*/
export const findChatMessageActionToClear = (messages: ChatMessage[], startNodeId?: string): ChatMessage | undefined => {
if (!startNodeId) return undefined
for (const message of messages) {
if (!message.action) continue
try {
const parsedAction = JSON.parse(message.action)
if (parsedAction?.data?.nodeId === startNodeId) return message
} catch (e) {
// error converting action to JSON, skip this message
}
}
return undefined
}
8 changes: 6 additions & 2 deletions packages/ui/src/views/chatmessage/ChatMessage.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -889,8 +889,12 @@ const ChatMessage = ({ open, chatflowid, isAgentCanvas, isDialog, previews, setP
setUserInput(elem.label)
setMessages((prevMessages) => {
let allMessages = [...cloneDeep(prevMessages)]
if (allMessages[allMessages.length - 1].type === 'userMessage') return allMessages
allMessages[allMessages.length - 1].action = null
// Clear the action on the message this button belongs to, not just the last message - a flow
// can have multiple pending HIL nodes at once. See #4787.
const targetIndex = allMessages.findIndex((msg) => msg.action && action && msg.action.id === action.id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Issue: Potential incorrect matching or runtime errors when comparing action IDs

There are two main concerns with the current comparison logic:

  1. Accidental matches on undefined: If both msg.action and action are objects but neither has an id property (or if they are both strings, making .id evaluate to undefined), msg.action.id === action.id will evaluate to true (since undefined === undefined is true). This can cause the wrong message's action to be cleared.
  2. Mixed types (String vs. Object): Depending on whether the message was loaded from history or newly streamed, msg.action and action can be either a JSON string or a parsed object. If one is a string and the other is an object, comparing .id directly will fail to match.

Recommendation

Implement a robust helper to safely extract the action ID, handling both stringified and parsed object formats, and explicitly verify that both IDs are defined before comparing them.

            const getActionId = (act) => {
                if (!act) return null
                if (typeof act === 'string') {
                    try {
                        return JSON.parse(act)?.id
                    } catch {
                        return null
                    }
                }
                return act.id
            }
            const targetActionId = getActionId(action)
            const targetIndex = allMessages.findIndex((msg) => {
                const msgActionId = getActionId(msg.action)
                return !!(msgActionId && targetActionId && msgActionId === targetActionId)
            })
References
  1. When using a heuristic for detection (e.g., for content type), ensure a safe fallback mechanism is in place to correctly handle cases where the heuristic fails.

const indexToClear = targetIndex !== -1 ? targetIndex : allMessages.length - 1
if (allMessages[indexToClear].type === 'userMessage') return allMessages
allMessages[indexToClear].action = null
return allMessages
})
if (elem.type.includes('agentflowv2')) {
Expand Down