Stop sending full defences on low levels #844
Conversation
pmarsh-scottlogic
requested review from
chriswilty,
AAloshine-scottlogic and
dhinrichs-scottlogic
pmarsh-scottlogic
changed the title
…fencecActivation test
|
Right. I've done a bunch more stuff
|
…s === undefined check with not currentDefences
|
I've restructured the tests in integration/defenceController.test. They now use the neat |
| @@ -240,14 +241,15 @@ async function handleChatToGPT(req: OpenAiChatRequest, res: Response) { | |||
| ? req.session.chatModel | |||
| : defaultChatModel; | |||
|
|
|||
| const defences = | |||
There was a problem hiding this comment.
I'm wondering why we want to define defences here (other than the system role) for levels 1 and 2, which leads me to wonder why handleChatWithoutDefenceDetection needs to receive the defences at all...
If you follow the passing of defences through handleChatWithoutDefenceDetection and beyond, you end up at handleAskQuestionFunction in src/openai.ts, which simply checks if the Q&A LLM defence is active. If we don't have any defences, then surely we can assume it's not active. Plus, it feels overly complex to be passing all the defences through so many calls, when all we need is the QA_LLM defence.
There was a problem hiding this comment.
what do you suggest? pass undefined through the calls instead? Make new methods for the whole call chain for getting a response without defences? I vote the former
There was a problem hiding this comment.
I suggest we don't pass defences into either handleChat function. The entire function call chain is self-contained, i.e. all functions are only invoked within this single call chain, so why not simply look up the defences from local storage in handleAskQuestionFunction? The currentLevel is passed all the way down so it's a trivial lookup. No need to pass defences down at all.
There was a problem hiding this comment.
The answer is: to get something from the session, we need access to the req object
There was a problem hiding this comment.
Of course, silly me! As discussed, let's try passing just the Q&A LLM defence down the call chain, but also have it optional so we don't need it for levels 1 and 2.
* defences undefined for levels 1 and 2 * fixes tests * propoerly undefines defences on level 1 and 2 * moves variable declarations to destructureing * missed one * undo error swallow * remove bad type guards * improve validation for configureDefence * last bit of validation * update tests for handleConfigureDefence * new folder for defence controller unit tests and wrote first handleDefencecActivation test * adds test for defence activation * adds tests for defence deactivation * moves tests all back into one file again * adds tests for handleResetSingleDefence * improve validation on handleGetDefenceStatus * removes extraneous stuff from test objects * adds tests to make sure defences aren't chngeable for level 1 * add loop to test levels 1 and 2 with same code * adds remaining tests for defence controller integration * handleResetSingleDefence takes level parameter now * adds level to frontend call to reset defence config item. Types all the prop drillings properly * some renamings and fill in gaps in tests * adds another missing test * tweaks * fix test suite * replace defence search with defences.some and replace cuurrentDefences === undefined check with not currentDefences * change defenceId for generic falsy check * improve guards for level and config * improve guards for level and config * correct test for configuring a config item * updates error message when not allowed to activate or modify defences * rescturecure defence controller integration tests * change other tests to use test.each rather than foreach * uses isValidLevel * improve validateFilterConfig * just pass qa defence rather than all defences * fix tests * uses type intersection to define QaLlmDefence * only gedDefencesFrDTOs if it is defined
Description
Level 1 and and 2 don't have defences. So they don't need defences stored in the backend session, nor do they need to be passed to the frontend. So I've got rid of 'em
Notes
Checklist
Have you done the following?