-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proposal-Police: GPT-4o Update with Structured Response #55108
base: main
Are you sure you want to change the base?
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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). |
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
merge
button 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
Dashboard
on top bar > thenAssistants
on 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 CONFIGURATION
section and set Response format tojson_schema
then add the following schema:JSON Schema - Structured Response
Save and you're all set ✅.
ℹ️ Review and Testing
System instructions
mentioned above in step (3) so we can adjust them before implementingActions
section, 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 Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop