-
Notifications
You must be signed in to change notification settings - Fork 30
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
fix: enhance shell() to know when it is interactive #66
Conversation
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.
Yes please
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.
This looks good, though I haven't tested it myself since I'm reviewing after hours.
src/goose/toolkit/developer.py
Outdated
pass | ||
|
||
# Check if no new lines have been received for 10 seconds | ||
if time.time() - last_line_time > 10: |
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.
Curious how this handles cases where log lines are replaced (e.g. print(..., end="\r"). This may be handled differently in stdout vs in a terminal console, but figured I'd float it as something to double check before merging.
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.
hrm - might be worth trying out
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 believe would timeout and then consider if it should continue or not
BTW have been using this all week and has been really great |
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.
This is super cool, great idea. I think we need to refactor how the subprocess is handled in the threads: if i'm understanding correctly, if the prompt is slow to show up i think we'll go through all the checks before anything is visible. That probably doesn't happen often - most prompts for input start immediately - but would like to take a pass to cover it
@baxen great - not sure if it really matters as a timeout would be triggered but wouldn't necessarily fail (but not sure how to confirm) - the accelerator would then decide or not if to continue (that could probably be enhanced?). There probably could be improvements so it isn't blocking on readline (in case there is no line feeds) and handling things that done return line by line perhaps, but overall, I haven't seen any specific failure modes that made it troublesome for me so far (would want to test it with some hefy tasks, like installs etc). |
We discussed a bit about the tradeoff between this occasionally being wrong - if the check model incorrectly identifies the command as stuck, it could interrupt something that was otherwise working. But this appears rare from testing, and seems worth it to catch the more common issue of running a web service |
Co-authored-by: Bradley Axen <baxen@squareup.com>
This will either return to user and ask them to run it themselves, or ask for more information, or work out how to run it with the information it has. Uses patterns and the accelerator model to supervise command output
fixes: #49
How this works:
acclerator
model to see if it may be a long running process and confirm with the user.For example, both of these are interactive commands now handled:
The former one uses the accelerator model to detect if it is long running or not. Former uses a pattern to detect.
Can also log stdout etc now (if we wanted a mode for that) - but you can just ask goose to show you