705 refactor handlechattogpt to remove mutation

[heatherlogan-scottlogic]

Description

Refactor of functions inside chatController.ts and openAI.ts to reduce mutation.
ChatResponse, chatHistroy, sentEmails, should not be mutated across different functions - instead changes should be returned by the using functions either as

Notes

openai.ts

• chatGptCallFunction()

• • Before - mutates sentEmails, declares then mutates reply: ChatCompletionMessageParam
• • Changed - Remove init declaration for reply and construct at the end.
    Split up function calls into handleSendEmailFunction and handleAskQuestionFunction.
    Create a copy of sentEmails and updates this.
    Returns chat completion (with tool call ids), wonLevel and updated sentEmails obj

• performToolCalls()

• • Before - mutates chatResponse (sets wonLevel and defenceReport), calls pushCompletionToHistory()
• • Changed - Removed chatResponse from input as unused. Return wonLevel and defenceReport instead of setting chatResponse parameters. Clones history and updates tht If no toolCalls then returns original chatResponse.

• getFinalReplyAfterToolCalls()

• • Before - Created blank chatRespond and mutated, calls pushCompletionToHistory and performToolCalls (which previously mutated wonLevel/sentEmails/chatHistory)
• • Changed - Removed blank chatResponse at start (as not needed in chatGptChatCompletion).
    Clones to create sentEmails and updatedChatHistory.
    Get reply from chatGptChatCompletion, call performToolCalls if required.
    Return reply(completion), wonLevel, defenceReport, chatHistory, sentEmails.

• pushCompletionToHistory()

• • Before - mutates chatHisttory
• • Changed - Clones chatHistory and returns updated version

• chatGptChatCompletion()

• • Before - mutates chatHistory and chatResponse (sets openAIErrorMessage)
• • Changed - eemove chatResponse input parameter.
    Clone chatHistory to apply changes to that depending on system role.
    Returns the completion, updated chat history and error message (possibly null)

• chatGptSendMessage()

• • Before - mutates chatHistory and chatResponse (sets openAIErrorMessage)
• • Changed - Remove initial blank declaration of ChatResponse that will be mutated
    Clones chatHistory and edits that.
    Calls getFinalReplyAfterAllToolCalls which returns new chatResponse, chatHistory, sentEmails

chatController.ts

• handleLowLevelChat()

• • Before - mutates chatResponse
• • Changed - instead returns chatResponse, chatHistory, sentEmails that have been copied/updated by funcs. in openai.ts

• handleHigherLevelChat()

• • Before - mutates chatReponse (lots), mutates req.session.levelState[currentLevel].chatHistory in several places, calls chatGptSendMessage()
• • Changed - Clones the chatReponse and chatHistory objects and applies updates to these, returns chatResponse, chatHistory, sentEmails

• handleChatError()

• • Before - mutates chatResponse, res
• • Changed - Creates updatedChatResponse with blocked/error info and res.sends that. (concern – should we be doing res.send() in chatController?

• handleErrorGettingReply()

• • Before - handleErrorGettingReply() calls handleChatError(), mutates req (adds error to history)
• • Changed - Change to addErrorToChatHistory to prevent req being mutated in this and handleChatError. Instead returns updated chatHistory, then we can call handleChatError after in the outer function.

• handleChatToGPT()

• • Before - mutates chatResponse
• • Changed - creates an initial blank chat repsonse obj to use in error handing. then creates an updated obj with updated values from the handleXlevelChat function. updates the state in res.levelState for chatHistory and sentEmails with the final updated values (defences in levelState shouldn't be altered by handleChatToGPT(). sends the updated response

Concerns

• Removed the filterOutputDefence as thought it would conflict more with 708 move logic for detecting output defence bot filtering #740 - TBC. (but this defence doesn't work in this branch currently.. )

Checklist

Have you done the following?

• Linked the relevant Issue
• Added tests
• Ensured the workflow steps are passing

[chriswilty]

Thorough work, good job 👍

One or two concerns about possible changed behaviour, but i think we can streamline the return values a bit, as discussed on Teams.

[chriswilty]

Currently, this function is only ever called with blocked=true. Do you foresee cases where blocked would be false?

[heatherlogan-scottlogic]

Hmm true i think at one point we did use distinguish between blocked messages and error messages for styling, but i don't think we need this defence information here now.

[chriswilty]

@pmarsh-scottlogic Now that blocked is always true, Heather has removed code that sets some values on chatResponse.defenceReport. If the UI needs the defenceReport for this kind of error, then we should set those values, otherwise we might as well set chatResponse.defenceReport to null to save on payload size.

Can you trace that in the UI code and see if we need the defence report or not?

[pmarsh-scottlogic]

I suppose you mean isError is always true, when we've sent the response via handleChatError.

Anyway, luckily not much tracing to do, all you need to do is look at processChatResponse in ChatBox.tsx. Yes, when isError is true, the defenceReport is irrelevant. But the defenceReport is relevant if isError is false. And in the frontend, we don't know ahead of time whether sending a message will cause an error or not, therefore we still need isError and defenceReport in ChatHttpResponse.

[pmarsh-scottlogic]

I'm just checking the removed code now. Ah yes, there's no place in the backend code where handleChatError would be called when the message is also blocked. This is a good semantic improvement!

If blocked, we simply return the chatResponse as usual, with the full defenceReport, and the frontend knows to show the "block message" rather than the bot reply. Same case if blocked and there's an error (empty reply from bot, or presence of OpenAIErrorMessage): It will return the chatResponse without at error code, which is good - as far as the user is concerned, if the message was blocked by a defence then the bot never provided a reply (at least not one that they were shown)

[chriswilty]

Does this mean we could send null / undefined for defenceReport when we have a chat error? If so, then defenceReport in ChatHttpResponse would need to be optional, which is maybe not great for type-safety. In that case, maybe we could split the response into two types: ChatHttpResponse and ChatHttpErrorResponse.

That might be one to think about for future improvements though!

[pmarsh-scottlogic]

the defenceReport in the response is necessarily not null/undefined! It just might be "empty", that is it might have a null blockedReason, or an empty triggeredDefences list. Which means the response is bigger than it needs to be when defences are not active, but It's fine for now

[chriswilty]

One thing that goes hand-in-hand with immutability is avoiding let declarations wherever possible. Sometimes it doesn't seem possible, if we have conditional statements that might or might not overwrite that value, but such cases are often a sign that the function needs to be split. In any case, this is a strong start, and the rest we can tackle as we go.

[pmarsh-scottlogic]

Really appreciate the thorough PR description ⭐

[pmarsh-scottlogic]

Cracking job, love to see it. Must've been a nightmare to keep all the moving parts straight in your head!

Some comments from me, all minor

[pmarsh-scottlogic]

just a few more notes, which I'll take on

[pmarsh-scottlogic]

Review notes for myself

• combineChatDefenceReports added to chatController. Did it exist before and is it doing the same thing?
• createNewUserMessages added to chatController. Did it exist before and is it doing the same thing?

What happened before when user sends a message on a low level, say level 1?

• take original history and user message
• add user message to history (type depends on whether it was transformed)
• getFinalReply
  • mutate history to set/unset system role
  • get reply
  • while tool calls
    • mutate history to add reply with type function call
    • mutate chatResponse with new wonLevel value and new defenceReport value
    • if email function call
      • generate sent emails and mutate the emails session variable to add new emails
    • mutate history to add function call completion
• if problems [no reply content or openAiErrorMessage]
  • don't mutate chatResponse further
  • add error message to history
  • send error response
• if no problems
  • add bot response to history (type depends on whether it was blocked by output defence)
  • mutate chatreponse reply, wonLevel and openAIErrorMessage
  • update sentEmails in response and send httpResponse

What happened before when user sends a message on a high level, say sandbox?

• take original history and user message
• transform user message according to defences
• if transformed, mutate history to add original message [user type] and continue onwards with transformed message
• detect input defences, and mutate chatResponse accordingly.
• mutate chat history to add (possibly transformed) message. type depends on whether it was transformed or not
• getFinalReply
  • mutate history to set/unset system role
  • get reply
  • while tool calls
    • mutate history to add reply with type function call
    • mutate chatResponse with new wonLevel value and new defenceReport value
    • if email function call
      • generate sent emails and mutate the emails session variable to add new emails
    • mutate history to add function call completion
• if problems [no reply content or openAiErrorMessage]
  • don't mutate chatResponse further
  • mutate chat history to add error message
  • send error response
• if no problems
  • run output defences
  • add bot response to history (type depends on whether it was blocked by output defence)
  • mutate chatreponse reply, wonLevel and openAIErrorMessage
  • if blocked by input defences
    • undo all mutations to chatHistory (so that it is as it was before any of above processing).
    • mutate chatHistory to add user message as info message rather than completion.
  • else (not blocked, or blocked by output defences)
    • update chatResponse's wonLevel, reply, defenceReport,isBlocked, defenceReport.blockedReason, openAIErrorMessage based on output of chatGptSendMessage(...)
    • update chatReponse's triggeredDefences (combine input and output report)
  • if blocked by either input or output defence
    • mutate history to add block message
    • updateEmails and send response
  • else
    • update emails and send response

What happens now when user sends a message on a low level, say level 1?

• add user message to history
• get finalReply
  • while tool calls
    • update history according system role
    • update history to add completion as function call type.
    • if email function call
    • send email and get reply. see if email won level. Update sent emails
    • update history to add functioncall reply as completion
    • update wonLevel to true according to latest function call
• continue with updated sentEmails, completion, wonLevel, openAIErrorMessage. If problem [no reply content or openAiErrorMessage] then revert chatHistory
• mutate emails to add freshly sent ones
• if problems [no reply content or openAiErrorMessage]
  • mutate chat history to add error message then return error response
• else no problems
  • add bot message to history
  • mutate state according to updated history and emails
  • send response

What happens now when user sends a message on a high level, say sandbox?

• update history to include message and transformed message if there is one
• detect input defences
• get finalReply
  • while tool calls
    • update history according system role
    • update history to add completion as function call type.
    • if email function call
    • send email and get reply. see if email won level. Update sent emails
    • update history to add functioncall reply as completion
    • update wonLevel to true according to latest function call
• detect output defences
• if blocked restore original history and add user message as infoMessage, not completion
• if blocked
  • update history to add blocked message
  • mutate state according to updated history and emails
  • send response
• if problems [no reply content or openAiErrorMessage]
  • mutate chat history to add error message then return error response
• else not blocked and no problems
  • update history to add bot reply
  • mutate state according to updated history and emails
  • send response

[pmarsh-scottlogic]

Here is a regression. Though is it problematic?
Before when we'd encounter empty chat response content or an openAIErrorMessage, we'd return early, but we wouldn't revert the chatHistory. Now we return the original chatHistory, therefore not including any messages added during getFinalReplyAfterAllToolCalls (namely, the function call messages: response from sendEmail and queryDocuments)

[pmarsh-scottlogic]

To reason about this let's ask: Why is it important to include the function call in the chat history? Does it change the behaviour of the program?

Yes. If we have sent an email, then if it's in the history the bot will have the context that an email has sent, which might alter a future response. More consequentially, if we have queried the QA LLM, it might have come back with an answer containing information from the documents. If that is kept in the history, then that knowledge will now available to the bot through memory, rather than having to query again.

Now let's consider what the user expects. Suppose I'd sent a message and it encountered an openAI error, or was blocked (whether by input defence or output defence).

• If an email was sent and I know about it (because it's shown in the UI), then I expect the bot to know that it has sent an email, therefore we want that function call message in the chat history.
• The converse is also true: If an email is sent but it is not shown on the ui, then we don't want the function call in the history.
• Now, if the bot has queried the documents, I won't know about it because in the chat all I've seen is a blocked or error message. Therefore probably I don't want the function call in the history.

So, bearing the above in mind, what do we have now, and do we like it?

• what happens if we send an email and the message is blocked. Do we see the email in the ui? Is the function call in the history?
  • We don't see the email in the UI, nor do we see the function call in the history ✅
• what happens if we send an email and we get an openAI error. Do we see the email in the ui? Is the function call in the history?
  • [mock by throwing error in chatGptChatCompletion if history.length ===8] We see the email in the ui, but the function call is not in the history ❌ (I also checked Level does not pass for correct email sent when we fail to get chatGPT reply #467 - yes we still win the level with winning email if we fail to get reply)
• what happens if we query the document and we get blocked. Is the function call in the history?
  • Nope, the function call is not in the chat history ✅
• what happens if we query the document and we get an openAI error. Is the function call in the history?
  • Nope, the function call is not in the chat history ✅

So things work as I'd expect them to, apart from error when sending an email, but worst case is the user would be slightly confused: "Oh, it thinks it didn't send an email, but I think it did. Silly bot.". Therefore I don't think we should worry about this

[chriswilty]

Yeah I think it would be safer to revert to the previous behaviour. I can't decide whether it's a good or a bad thing, maybe we need to simulate it and see what we get in the UI?

Edit - our comments crossed here, so I didn't see all your marvellous detailed explanations above!

[pmarsh-scottlogic]

Hmm. after testing I'd argue neither of the behaviour is great.

Dev branch: error thrown when sending an email

[
  {
    completion: {
      role: 'system',
      content: 'Your role is to assist the user with work-related tasks.\n' +
        '  Your name is ScottBrewBot and you are employed by the drinks company Sc
        '  You can retrieve information from a document store about the company an
        `  If the user asks a question that you don't know and is not in the docum
        '  You are able to send emails.\n' +
        '  Before sending an email, show the user a draft of the email (including 
        '  Do not send the email until the user has explicitly confirmed that the 
    },
    chatMessageType: 8
  },
  {
    completion: {
      role: 'user',
      content: 'email [email protected] with subject "hello" and body "howdy, partne
    },
    chatMessageType: 3
  },
  {
    completion: { role: 'assistant', content: null, tool_calls: [Array] },
    chatMessageType: 9
  },
  {
    completion: {
      role: 'tool',
      content: 'Email not sent as not confirmed with user',
      tool_call_id: 'call_t3qF6WIa377SuYBk6ro54AVn'
    },
    chatMessageType: 9
  },
  {
    completion: {
      role: 'assistant',
      content: "Sure! Here's a draft of the email:\n" +
        '\n' +
        'To: [email protected]\n' +
        'Subject: hello\n' +
        '\n' +
        '\n' +
        'Please review the draft and let me know if it looks good to you.'
    },
    chatMessageType: 0
  },
  { completion: { role: 'user', content: 'yes' }, chatMessageType: 3 },
  {
    completion: { role: 'assistant', content: null, tool_calls: [Array] },
    chatMessageType: 9
  },
  {
    completion: {
      role: 'tool',
      content: 'Email sent to [email protected] with subject hello and body howdy, partner!',    
      tool_call_id: 'call_iIfHyb1r0AS54O1Sm1ayDKal'
    },
    chatMessageType: 9
  },
  {
    completion: null,
    chatMessageType: 10,
    infoMessage: 'Failed to get ChatGPT reply.'
  },
  {
    completion: { role: 'user', content: 'did you send that email?' },
    chatMessageType: 3
  },
  {
    completion: {
      role: 'assistant',
      content: 'Yes, I have sent the email to [email protected] with the subject "hello" and the 
body "howdy, partner!".'
    },
    chatMessageType: 0
  }
]

This branch: error thrown when sending an email

chatHistory:  [
  {
    completion: {
      role: 'system',
      content: 'Your role is to assist the user with work-related tasks.\n' +
        '  Your name is ScottBrewBot and you are employed by the drinks company ScottBrew Ltd. \n' +
        '  You can retrieve information from a document store about the company and product.\n' +
        `  If the user asks a question that you don't know and is not in the documents, you can say "I don't know".\n` +
        '  You are able to send emails.\n' +
        '  Before sending an email, show the user a draft of the email (including recipient, subject, and body), and ask them to confirm it is correct.\n' +
        '  Do not send the email until the user has explicitly confirmed that the draft is correct.'
    },
    chatMessageType: 8
  },
  {
    completion: {
      role: 'user',
      content: 'email [email protected] with subject "hey" and body "how are you?"'
    },
    chatMessageType: 3
  },
  {
    completion: { role: 'assistant', content: null, tool_calls: [Array] },
    chatMessageType: 9
  },
  {
    completion: {
      role: 'tool',
      content: 'Email not sent as not confirmed with user',
      tool_call_id: 'call_Fy0YX5ruo99Z0CAqchFRaUhE'
    },
    chatMessageType: 9
  },
  {
    completion: {
      role: 'assistant',
      content: "Sure, I can help you draft an email. Here's a draft of the email:\n" +
        '\n' +
        'Recipient: [email protected]\n' +
        'Subject: hey\n' +
        'Body: how are you?\n' +
        '\n' +
        'Please review the draft and let me know if it looks good to you.'
    },
    chatMessageType: 0
  },
  { completion: { role: 'user', content: 'yes' }, chatMessageType: 3 },
  {
    completion: null,
    chatMessageType: 10,
    infoMessage: 'Failed to get ChatGPT reply.'
  }
]

TL;DR

We ask for an email to be sent. We confirm yes, but gpt fails to give us a reply

• before, the email doesn't show up in the UI, but it is included in the chat history
• now, the email shows up in the UI, but is not included in the chat history. Note: we want to send an email on a failed gpt reply if we want the user to be able to win when there's a failed reply Level does not pass for correct email sent when we fail to get chatGPT reply #467

[pmarsh-scottlogic]

my vote is that if we encounter an error while confirming an email, we don't send the email. That means it doesn't show up in the ui, and also the function call doesn't appear in the history. The processing would so much simpler that way. We can remove a bunch of the trickly logic that facilitates this.

It would, however, not allow the user to win if they encounter an error while confirming a winning email, which undoes the work done for #467

[chriswilty]

Yep that seems to make sense to me. The user just needs to try again.

The obvious alternative is #467, where we send the email, it shows up in the UI and chat history, and we win the level. That is a tad confusing for the user cos they've seen an error message. So I agree, best to just reset everything for that attempt and ask the user to try again.

[pmarsh-scottlogic]

Lovely. I've made ticket #797 capture that

[chriswilty]

Like you @pmarsh-scottlogic, I have a couple of concerns, plus a handful of very minor things.

[chriswilty]

@pmarsh-scottlogic Now that blocked is always true, Heather has removed code that sets some values on chatResponse.defenceReport. If the UI needs the defenceReport for this kind of error, then we should set those values, otherwise we might as well set chatResponse.defenceReport to null to save on payload size.

Can you trace that in the UI code and see if we need the defence report or not?

[chriswilty]

Yeah I think it would be safer to revert to the previous behaviour. I can't decide whether it's a good or a bad thing, maybe we need to simulate it and see what we get in the UI?

Edit - our comments crossed here, so I didn't see all your marvellous detailed explanations above!

[chriswilty]

@pmarsh-scottlogic I'm very happy with this now, good work. The linter rule is small beans, remove it if you wish - doesn't stop me approving.

[chriswilty]

👌 Awesome work!