Proposal-Police: GPT-4o Update with Structured Response#55108
Conversation
|
@dominictb Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@dominictb This will not require C+ review. Note for reviewersThe reason for changes in all the other non-proposal-police related GH action files is because of the changes in |
thienlnam
left a comment
thienlnam
left a comment
There was a problem hiding this comment.
Looking good so far, wondering if we can simplify the response even more
| const isNoAction = action.trim().toUpperCase() === CONST.NO_ACTION; | ||
| const isActionRequired = action.trim().toUpperCase() === CONST.ACTION_REQUIRED; |
There was a problem hiding this comment.
NAB - we define the possible values for the action so it shouldn't be necessary to trim / case match the values
There was a problem hiding this comment.
I wanted to keep this from the old logic just in case the response comes lowercased but given the structured response json_schema enums only allow uppercase as specified, I guess toUpperCase() can be safely removed.
| // If assistant response is NO_ACTION and there's no message, do nothing | ||
| if (isNoAction && !message) { | ||
| console.log('Detected NO_ACTION for comment, returning early.'); | ||
| return; | ||
| } | ||
|
|
||
| // if the assistant responded with no action but there's some context in the response | ||
| if (assistantResponse.includes(`[${CONST.NO_ACTION}]`)) { | ||
| // extract the text after [NO_ACTION] from assistantResponse since this is a | ||
| // bot related action keyword | ||
| const noActionContext = assistantResponse.split(`[${CONST.NO_ACTION}] `).at(1)?.replace('"', ''); | ||
| console.log('[NO_ACTION] w/ context: ', noActionContext); | ||
| // if the assistant responded with no action but there's some context in the message | ||
| if (isNoAction && !!message) { | ||
| console.log('[NO_ACTION] with Message: ', message); |
There was a problem hiding this comment.
I don't think we need to check this and can just validate through the action - from previous usages I have found that unless you're extremely explicit about it not adding a message if action is NO_ACTION it will still add a message but it's fine since we have them choose the action explicitly
| } else if (assistantResponse.includes('[EDIT_COMMENT]') && !payload.comment?.body.includes('Edited by **proposal-police**')) { | ||
| // extract the text after [EDIT_COMMENT] from assistantResponse since this is a | ||
| // edit comment if assistant detected substantial changes | ||
| } else if (isActionRequired && message.includes('[EDIT_COMMENT]')) { |
There was a problem hiding this comment.
Are we having EDIT_COMMENT included in the message still? Should this just be the GH comment and have a separate action for ACTION_EDIT?
There was a problem hiding this comment.
Sure, will create a new action and test it on my clone first to make sure it works as expected.
|
@thienlnam Just pushed the requested changes and also:
to comply with the request from #55108 (comment). |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
|
Updated the assistant 🚀 sending it |
This comment was marked as outdated.
This comment was marked as outdated.
|
@thienlnam Did you update the AI assistant as requested in PR description following all steps ? Asking because here's a recent run (post PR) where it looks like the parsed response is NO_ACTION (string), meaning it didn't return structured response which caused JSON.parse error and bot wouldn't return anything when there's such an error. Note that besides the system instructions, I also added mandatory section which enforces structured response via |
|
I did update the response format, and checked again and it is still json_schema - is this the correct assistant? asst_A2nLg9DUrqi4MCH3PQXr5sx2 |
|
Hmm, weird. Maybe it takes some time to update 🤷 @thienlnam Should be the correct assistant since the GH action takes the ID automatically from App/.github/workflows/proposalPolice.yml Lines 19 to 21 in 7b8bb55 |
|
Ah it seems like it's actually a different assistant being used - cc @justinpersaud / @marcochavezf since I think you help set this up - if you have access to this assistant asst_ITdeb3p87GYwbfNvRAA5heGX, could you update it with the instructions in the PR description? |
|
@thienlnam Got it, makes sense now - since from my tests the AI assistant updates usually apply instantly. If the people you tagged are not around right now, you could create a new assistant with the instructions and response schema detailed in PR description and replace the ID in GitHub secrets Important Either way works as long as the exact steps from PR description are applied to the assistant. Right now we still get old non-structured response which causes JSON.parse to error and bot won't do anything after that since we designed it to fail gracefully, meaning action will still be completed (green) unless GH is down or something else non-related to the proposal-police code. We implemented graceful failing since otherwise the GH action would notify all people participating in the issue which we want to avoid (spam). |
|
🚀 Deployed to staging by https://github.com/thienlnam in version: 9.0.85-0 🚀
|
1 similar comment
|
🚀 Deployed to staging by https://github.com/thienlnam in version: 9.0.85-0 🚀
|
|
@thienlnam I updated the assistant per instructions |
|
@ikevin127 Do we need QA testing this? |
|
🚀 Deployed to staging by https://github.com/thienlnam in version: 9.0.85-0 🚀
|
1 similar comment
|
🚀 Deployed to staging by https://github.com/thienlnam in version: 9.0.85-0 🚀
|
|
🚀 Deployed to staging by https://github.com/thienlnam in version: 9.0.85-0 🚀
|
|
Sorry, wasn't around but we should be all good now |
|
Looks good, instructions updated and returning structured responses now 👍 |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.85-4 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.85-4 🚀
|
4 participants
Explanation of Change
This PR is an update for proposal police GH action which will match with the updated gpt-4o model instructions where the AI assistant will now return structured responses.
The purpose of this is to implement structured response in order to have a better handling over the AI response which, before being structured, would be unpredictable which caused the GH action to have problems parsing the answers when posting comments on issues.
Fixed Issues
$ #54980
PROPOSAL:
cc @thienlnam @marcochavezf
Caution
🛑 Important
Right before pressing the
mergebutton on this PR, we need to ensure that the AI Assistant is configured with the updated instructions and structured response settings to match the GH action code changes from this PR.♻️ OpenAI Dashboard - Proposal Police AI Assistant Update Steps
Login on Expensify's OpenAI Platform @ https://platform.openai.com.
Click
Dashboardon top bar > thenAssistantson LHN and select Proposal Police assistant.Replace the current System instructions with the new ones:
Updated instructions (please review)
Ensure the selected Model is
gpt-4o.Scroll down to the
MODEL CONFIGURATIONsection and set Response format tojson_schemathen add the following schema:JSON Schema - Structured Response
{ "name": "action_schema", "strict": true, "schema": { "type": "object", "properties": { "action": { "type": "string", "enum": [ "NO_ACTION", "ACTION_EDIT", "ACTION_REQUIRED" ], "description": "Indicates the action type." }, "message": { "type": "string", "description": "An optional template message that can be empty or specified." } }, "required": [ "action", "message" ], "additionalProperties": false } }Save and you're all set ✅.
ℹ️ Review and Testing
System instructionsmentioned above in step (3) so we can adjust them before implementingActionssection, as well as post different variations of proposal / non-proposal comments on this issue where I already performed testingcc @thienlnam @marcochavezf
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop