fix: replace print() with logging and bare excepts in graph_debugger.py#342
fix: replace print() with logging and bare excepts in graph_debugger.py#342Samarth0211 wants to merge 1 commit intomicrosoft:mainfrom
Conversation
Fixes microsoft#334 and microsoft#332 - Replace all library-internal print() calls with logger.warning() and logger.info() using the module-level logger = logging.getLogger(__name__) - CLI-facing print() in session_commands.py is intentionally left as-is per issue guidance ("CLI user-facing print is OK") - Replace bare `except:` blocks with `except Exception:` in _generate_static_png() and generate_comparison_visualization() to avoid silently catching KeyboardInterrupt and SystemExit - Add `import logging` and module-level logger at top of file
|
Welcome to the Agent Governance Toolkit! Thanks for your first pull request. |
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This pull request addresses two issues: replacing print() statements with proper logging and replacing bare except: blocks with except Exception: in the graph_debugger.py file. These changes are aligned with Python best practices and improve the maintainability and robustness of the code. However, there are a few areas that require attention to ensure correctness, security, and maintainability.
Key Findings
🔴 CRITICAL: Logging Sensitive Information
- Potential Logging of Sensitive Data:
- In the
_get_node_stylemethod, thetitlestring includestrace.validation_errors. Ifvalidation_errorscontains sensitive information (e.g., user data, credentials, or other sensitive details), logging this data usinglogger.infoorlogger.warningcould lead to information leakage. - Recommendation: Ensure that
trace.validation_errorsdoes not contain sensitive information. If it might, sanitize or redact sensitive data before logging.
- In the
💡 SUGGESTION: Improve Logging Messages
-
Generic Logging Messages:
- Some logging messages, such as
"Visualization libraries not available: %s. Install with: pip install matplotlib networkx pyvis", could be more specific about the impact of missing libraries on the functionality. - Recommendation: Update the message to include the consequences of missing libraries (e.g., "Visualization libraries not available: %s. Visualization features will be disabled. Install with: pip install matplotlib networkx pyvis").
- Some logging messages, such as
-
Logging Levels:
- The use of
logger.warningfor missing libraries is appropriate. However, consider usinglogger.errorfor critical issues like unsupported formats in thevisualize_tracemethod. - Recommendation: Use
logger.errorfor cases where the program cannot proceed due to an error.
- The use of
💡 SUGGESTION: Exception Handling
- Generic Exception Handling:
- While replacing
except:withexcept Exception:is an improvement, it is still a broad exception handling practice. For example, in_generate_static_png, theexcept Exception:block could mask unexpected errors. - Recommendation: Narrow down the exception types to handle only expected exceptions (e.g.,
ValueError,KeyError, etc.). If a broad exception is necessary, log the exception details usinglogger.exceptionto aid debugging.
- While replacing
💡 SUGGESTION: Logging Configuration
- Logging Configuration:
- The PR introduces a
loggerobject but does not configure the logging module. This could lead to logs being silently dropped if no handlers are configured. - Recommendation: Add a default logging configuration in the main entry point of the application or library (e.g.,
logging.basicConfig(level=logging.INFO)).
- The PR introduces a
💡 SUGGESTION: Code Formatting
- Code Formatting Issues:
- The diff shows inconsistent indentation, with some lines using 4 spaces and others using 8 spaces. This inconsistency makes the code harder to read and maintain.
- Recommendation: Run a linter (e.g.,
ruff) or a code formatter (e.g.,black) to ensure consistent formatting.
💡 SUGGESTION: Testing
- Testing for Logging and Exception Handling:
- The PR does not include any new tests to verify the changes made to logging and exception handling.
- Recommendation: Add tests to ensure that:
- Logging messages are generated as expected.
- The application behaves correctly when exceptions are raised (e.g., by mocking the logger and asserting the logged messages).
Final Assessment
- The changes in this PR improve the codebase by replacing
print()with logging and addressing the use of bareexcept:blocks. However, there are some areas that require further attention, particularly around logging sensitive information and exception handling. - The PR does not introduce any breaking changes, but the potential for sensitive data leakage in logs is a critical issue that must be addressed.
Actionable Feedback Summary
- 🔴 CRITICAL: Ensure that sensitive information (e.g.,
trace.validation_errors) is not logged or is properly sanitized/redacted before logging. - 💡 SUGGESTION: Improve the specificity and clarity of logging messages, especially for critical issues.
- 💡 SUGGESTION: Replace broad
except Exception:blocks with more specific exception handling where possible. Uselogger.exceptionto log details of unexpected errors. - 💡 SUGGESTION: Add a default logging configuration to ensure logs are not silently dropped.
- 💡 SUGGESTION: Run a linter (e.g.,
ruff) or formatter (e.g.,black) to fix inconsistent indentation and ensure code style compliance. - 💡 SUGGESTION: Add tests to verify the correctness of logging and exception handling changes.
Let me know if you need further clarification or assistance!
|
@Samarth0211 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
🤖 AI Agent: contributor-guideHello @first-time-contributor! 👋 Welcome to the microsoft/agent-governance-toolkit project, and thank you for taking the time to contribute! 🎉 We’re thrilled to have you here, and we appreciate your effort in addressing issues #334 and #332. Let’s dive into your pull request together! What You Did Well ✅
Suggestions for Improvement ✨
Next Steps 🚀
After you’ve made these changes, we’ll review your PR again. If you have any questions or need help, feel free to ask! We’re here to support you. 😊 Thank you again for contributing to the project — we’re excited to collaborate with you! 🎉 |
🤖 AI Agent: security-scannerSecurity Review of PR:
|
Fixes #334 and #332
except:blocks withexcept Exception:in _generate_static_png() and generate_comparison_visualization() to avoid silently catching KeyboardInterrupt and SystemExitimport loggingand module-level logger at top of fileDescription
Type of Change
Package(s) Affected
Checklist
Related Issues