-
Notifications
You must be signed in to change notification settings - Fork 7.3k
feat: enhance --sysinfo to include version and masked environment var… #1336
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
base: main
Are you sure you want to change the base?
feat: enhance --sysinfo to include version and masked environment var… #1336
Conversation
…iables - Add get_gpt_engineer_version() to detect version and installation type (released vs dev) - Add get_environment_variables() to show relevant env vars with sensitive data masked - Add mask_sensitive_value() to securely mask API keys and secrets - Update get_system_info() to include version and environment information - Improve sysinfo output formatting with better structure - Add comprehensive unit tests for all new functionality This enhancement helps with: - Version tracking for better issue diagnosis - Environment configuration insight while maintaining security - Security compliance by masking sensitive information Fixes: Issue requesting enhanced --sysinfo output
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.
Caution
Changes requested ❌
Reviewed everything up to 7e7b795 in 2 minutes and 18 seconds. Click for details.
- Reviewed
424lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. gpt_engineer/applications/cli/main.py:1
- Draft comment:
No license specification update found in this diff. Please include/update the LICENSE file as requested. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. gpt_engineer/applications/cli/main.py:311
- Draft comment:
Consider expanding the docstring to clarify how wildcard patterns (e.g. 'GPT_ENGINEER_*') are handled in get_environment_variables. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a documentation/docstring improvement suggestion. The rules state "Do NOT comment unless there is clearly a code change required" and "Do NOT make comments that are obvious or unimportant." The wildcard handling is already explained in an inline comment in the code itself (line 311), and the implementation is straightforward. The docstring already describes what the function does ("Get relevant environment variables with sensitive data masked"). Adding more detail about wildcard patterns to the docstring is a nice-to-have but not a required code change. This falls into the category of optional documentation improvements rather than necessary code fixes. The wildcard pattern handling might not be immediately obvious to users of this function just from reading the docstring. If someone is trying to understand what environment variables will be collected, knowing that wildcards are supported could be helpful information. However, the inline comment in the code already documents this. While the additional documentation could be helpful, the rules explicitly state not to comment unless there's a clear code change required. This is a documentation enhancement, not a bug fix or necessary change. The inline comment already documents the wildcard behavior for anyone reading the code. The function works correctly as-is. This comment should be deleted. It's a documentation improvement suggestion rather than a required code change. The wildcard behavior is already documented via inline comments in the code, and the function works correctly.
3. tests/applications/cli/test_sysinfo.py:17
- Draft comment:
Excellent and comprehensive tests for mask_sensitive_value covering long, short, empty, and sensitive cases. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. tests/applications/cli/test_sysinfo.py:120
- Draft comment:
Good use of patching to simulate package metadata in get_gpt_engineer_version tests; covers both released and development versions. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_VJyRErbKteWC7Z7z
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| try: | ||
| metadata = importlib.metadata.metadata("gpt-engineer") | ||
| # Check if installed in editable mode or from source | ||
| installer = metadata.get("Installer", "unknown") |
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.
Unused variable installer in get_gpt_engineer_version; consider removing it if not needed.
| installer = metadata.get("Installer", "unknown") |
- Removed unused 'installer' variable that was retrieved but never used - Updated tests to remove unnecessary metadata mocking - Addresses code review feedback from Ellipsis bot
Enhance --sysinfo to include version and masked environment variables
🎯 Description
This PR enhances the
--sysinfooption to provide more comprehensive debugging information by adding version tracking and environment variable visibility while maintaining security through sensitive data masking.Addresses the feature request for improved system information output to help diagnose issues more effectively.
✨ Changes
New Functions Added
get_gpt_engineer_version().dev,.post, and git hash suffixesget_environment_variables()mask_sensitive_value()sk-p...cdef (length: 24))****** (length: X)for shorter sensitive valuesEnhanced Features
get_system_info()to include version and environment information🧪 Testing
tests/applications/cli/test_sysinfo.py📊 Example Output
Before:
After:
🎁 Benefits
Version Tracking
Environment Configuration Insight
Security Compliance
📝 Files Changed
gpt_engineer/applications/cli/main.py(+385 lines)reimport for pattern matchingtests/applications/cli/test_sysinfo.py(new file, +269 lines)🔍 Breaking Changes
None. This PR only adds new functionality to the existing
--sysinfooption without modifying any existing behavior.✅ Checklist
mainbranch🔗 Related Issues
This PR is labeled as a good first issue contribution.
Testing Instructions:
To test this PR locally:
Important
Enhances
--sysinfoto include version and masked environment variables, with new functions and comprehensive tests added.--sysinfoto include version and masked environment variables.get_gpt_engineer_version()to detect version and installation type.get_environment_variables()to capture and mask environment variables.get_system_info()to include new information.mask_sensitive_value()masks sensitive data in environment variables.main.pyfor better readability.test_sysinfo.pywith comprehensive unit tests for new functions.reimport inmain.pyfor pattern matching.This description was created by
for 7e7b795. You can customize this summary. It will automatically update as commits are pushed.