-
-
Notifications
You must be signed in to change notification settings - Fork 211
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: properly handle piped input in prompts #354
Conversation
This PR improves upon #344 which added initial support for piped stdin. Instead of treating piped input as a separate message, it now properly integrates it into the first prompt message (or creates a new one if none exists), making the behavior more consistent and intuitive. |
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.
❌ Changes requested. Reviewed everything up to 040622b in 1 minute and 12 seconds
More details
- Looked at
47
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_o043PHngYohJeCKQ
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
gptme/cli.py
Outdated
@@ -283,6 +294,7 @@ def main( | |||
) | |||
except RuntimeError as e: | |||
logger.error(e) |
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.
logger.error(e)
is redundant since logger.exception(e)
already logs the error message. Consider removing logger.error(e)
. This applies to line 296 and 297.
Previously, piped input was treated as a separate message instead of being included in the user prompt. This change: - Adds inject_stdin function to handle piped input - Ensures piped input is appended to first prompt or creates new message if none exists - Removes redundant system message creation
040622b
to
57cdc9f
Compare
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.
👍 Looks good to me! Incremental review on 57cdc9f in 10 seconds
More details
- Looked at
39
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. gptme/cli.py:241
- Draft comment:
Consider movinginject_stdin
outside themain
function if it needs to be reused elsewhere. However, the current implementation is correct as per the PR description. - Reason this comment was not posted:
Confidence changes required:33%
Theinject_stdin
function is defined within themain
function, which is fine for encapsulation, but it could be moved outside if it needs to be reused elsewhere. However, the current implementation is correct as per the PR description.
Workflow ID: wflow_utoXWTgQfPo84pex
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #354 +/- ##
==========================================
- Coverage 71.15% 70.86% -0.30%
==========================================
Files 68 68
Lines 5228 5234 +6
==========================================
- Hits 3720 3709 -11
- Misses 1508 1525 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Issue
Previously, piped input was being treated as a separate message instead of being included in the user prompt. This could lead to unexpected behavior when using gptme with piped input.
Changes
Testing
Tested with various piped input scenarios:
Important
Introduces
inject_stdin
function ingptme/cli.py
to handle piped input by appending it to the first prompt or creating a new message, removing redundant system message creation.inject_stdin
function ingptme/cli.py
to handle piped input.This description was created by for 57cdc9f. It will automatically update as commits are pushed.