-
Notifications
You must be signed in to change notification settings - Fork 45.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
feat(iffy): Add initial iffy moderation #9621
base: dev
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for auto-gpt-docs-dev canceled.
|
Here's the code health analysis summary for commits Analysis Summary
|
✅ Deploy Preview for auto-gpt-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
The PR appears to have several issues that need addressing: 1) The test plan section is completely empty when it should document testing steps for such a critical security feature 2) The PR template is only partially filled out - missing configuration checklist items which may be relevant given new environment variables being added 3) The PR acknowledges known issues with race conditions and incomplete moderation coverage that could lead to unsafe content getting through. While the core functionality seems solid, these gaps should be addressed before merging. |
The PR follows most guidelines but has some issues that need addressing: 1) The test plan is completely empty and marked as skipped rather than explaining why testing may be difficult, 2) While they state configuration changes are needed for Iffy/GCP, the configuration checklist section is missing entirely rather than being filled out to indicate needed changes to .env.example etc. |
The PR is well documented with clear changes and explanations, but has some issues to address: 1) The test plan is incomplete and they acknowledge testing will be difficult for others, 2) There are known execution timing issues that need to be resolved, 3) Missing runtime block checking capability. However, the code changes themselves properly handle user_id validation and have good security considerations built in with the fallback to OpenRouter when Iffy fails. The scope of changes is focused on content moderation features. |
The PR appears to be well documented with clear changes and explanations. The author has acknowledged some current limitations and areas for improvement. However, there are a few issues that need to be addressed: 1) The test plan is empty and marked as difficult to test locally 2) The author notes some timing issues with iffy vs executor that need to be resolved 3) The PR includes significant new functionality but does not have proper test coverage plans. While these are concerns, they appear to be actively acknowledged and discussed rather than ignored. |
While this PR shows good progress on implementing content moderation, there are a few issues that need to be addressed:
|
asyncio.set_event_loop(loop) | ||
|
||
# Run the async function in the event loop | ||
return loop.run_until_complete( |
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.
self.run_and_wait(
probably exists
This is the initial code for the iffy moderation setup.
Changes 🏗️
In backend/executor/manager.py I have had to make it so add_execution is synchronous but calls an async implementation because we need to perform content moderation with send_to_iffy, which is an async function, if there is a better way to do this please let me know, Then further down we collect all the blocks and the info and send it all to iffy
In backend/util/iffy.py this is where we have the actual
send_to_iffy
function, this gets all of the data and formats it + gets the other data we need like username and userid + adds the metadata likegraphExecutionId
ect, then combines it all together into a payload to be sent to iffyIn backend/server/v2/iffy/routes.py i have added
@iffy_router.post("/webhook")
to receive the returned info from iffy's webhook, then we split it into two.handle_record_event
deals with flagged blocks, if any are flagged it gets thegraph_exec_id
from the metadata, then callsexecution_manager.cancel_execution(graph_exec_id)
to stop the graph from running.and
handle_user_event
, currently we dont have any systems setup yet to deal with suspending/banning a user so for now i just log the data and return 200 to prevent iffy from sending the data again. The next TODO: would be to setup a system to actually use this info so we can suspend/ban usersI have added a fallback / backup incase iffy fails to reply/if iffy is down in backend/util/openrouter.py we are just using OpenRouter and making it pick any model and just asking it if it things the content is safe or not, if flagged it stops the graph from running. TODO: Improve this and the prompting down the line
In backend/server/rest_api.py i am just adding the router for
/api/iffy
This current setup does work but there are some issues that i have been trying to fix and i feel some other eyes on this may help.
List of TODO's down the line/in other Pr's:
Once we are happy with this setup we will need to setup Iffy on GCP.
Link to Iffy's github and also big shout out to @s3ththompson and the Iffy team for several updates to iffy during the setup of this which has helped so much. Id also love your feedback on this if you have any thoughts or suggestions.
Checklist 📋
This may be difficult to test locally for other people just because of how i have iffy setup, i am hosting a version of it on my server, i can give the api keys/url's and so on just contact me, tho you may need to use nginx? to make it so iffy can call back to your local machine for its webhook replies, For now ill leave the test plan empty because of this
For code changes: