-
Notifications
You must be signed in to change notification settings - Fork 6
Notebook tools #124
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
Notebook tools #124
Conversation
ellisonbg
left a 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.
Early review comments.
| args = command | ||
|
|
||
| # Check if the command is in the whitelist | ||
| if not args or args[0] not in ALLOWED_COMMANDS: |
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.
It is too easy to subvert this check for it to be useful. I think we are better off just asking for human approval.
| ) | ||
|
|
||
| # Execute the command | ||
| process = subprocess.Popen( |
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.
Let's make this all async and use https://docs.python.org/3/library/asyncio-subprocess.html
|
|
||
| # Parse the output if successful | ||
| if result["returncode"] == 0 and result["stdout"]: | ||
| entries = [] |
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 think we may be better off just returning the text outputs given that the output will be used by a future call to a model rather than Python code.
|
Closing, moved the tools to jupyter-ai-contrib/jupyter-ai-tools#3 |
No description provided.