-
Notifications
You must be signed in to change notification settings - Fork 45
Fix: exclude commercial-existing fields that violates pydantic #428
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?
Conversation
Summary of ChangesHello @haggit-eliyahu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the data validation mechanisms within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
508960e to
02aa5f2
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.
Code Review
This pull request introduces several fixes to handle validation issues with existing commercial integrations. This includes excluding certain parameter descriptions from length validation, adding numerous exceptions to regex patterns for script and parameter names, increasing the maximum word count for parameter names, and providing a default value for a missing field. The approach of adding exceptions is pragmatic for dealing with legacy data. The implementation of the new validator and its application across various data models is well-executed. I have identified a potential bug in one of the new regex patterns and some redundant entries that should be cleaned up for better maintainability.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/mp/src/mp/core/constants.py (476)
This regex pattern appears to contain a typo and might be the result of a copy-paste error.
- The part
Google Rapid Response \(GR$seems incomplete and is likely intended to beGoogle Rapid Response \(GRR\). However, an exclusion forGoogle Rapid Response (GRR)already exists on line 468. - The second part of the alternation,
Tenable\.io - List Endpoint Vulnerabilities$, is missing a^at the beginning, which means it would match any string ending with that text, not the exact string. This is inconsistent with other patterns in this list.
This looks like it should just be an exclusion for the Tenable script name.
r"|^Tenable\.io - List Endpoint Vulnerabilities$"
packages/mp/src/mp/core/constants.py (560-606)
There are several duplicated patterns in this list of exceptions for PARAM_DISPLAY_NAME_REGEX. While this doesn't break the functionality of the regular expression, it adds unnecessary clutter and makes the list harder to read and maintain.
For example:
r"|^Fetch\ Backwards\ Time\ Interval\ \(minutes\)$\"appears 5 times.r"|^Extract\ urls\ from\ HTML\ email\ part\?\$"appears 3 times.r"|^Create\ a\ Separate\ Siemplify\ Alert\ per\ Attached\ Mail\ File\?\$"appears 3 times.
Please remove the duplicate entries to improve code clarity.
02aa5f2 to
51d4d16
Compare
|
/gemini review |
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.
Code Review
This pull request aims to fix validation issues for commercial integrations by adding exclusion lists and a new validator for parameter descriptions. The changes correctly apply the new validator for various parameter types. However, there's a critical issue where this new parameter-specific validator is incorrectly applied to action descriptions, which have a different length constraint, potentially breaking validation for valid data. Additionally, the growing exclusion lists added to the constants file are a maintainability concern; I've suggested moving them to separate data files to keep the codebase cleaner.
# Conflicts: # packages/mp/pyproject.toml # packages/mp/uv.lock
a502a5a to
c1e543b
Compare
|
/gemini review |
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.
Code Review
This pull request introduces a significant and beneficial refactoring by moving hardcoded exclusion lists from Python constants into a centralized exclusions.yaml file. This greatly improves maintainability. A new exclusions.py module with cached loading is a solid implementation for accessing this configuration. The PR also successfully addresses the pydantic validation issues by adding custom validators for parameter descriptions and increasing the word limit for parameter names.
My review includes a couple of suggestions to fix docstrings in the new validator functions and to remove some redundant JSON files that were added, which could cause confusion.
packages/mp/src/mp/core/validation_data/excluded_long_param_description_prefixes.json
Outdated
Show resolved
Hide resolved
packages/mp/src/mp/core/validation_data/excluded_param_display_names.json
Outdated
Show resolved
Hide resolved
packages/mp/src/mp/core/validation_data/excluded_script_display_names.json
Outdated
Show resolved
Hide resolved
1984c60 to
d00d765
Compare
Description
Checklist:
Please ensure you have completed the following items before submitting your PR.
This helps us review your contribution faster and more efficiently.
General Checks:
Open-Source Specific Checks:
For Google Team Members and Reviewers Only:
Screenshots (If Applicable)
If your changes involve UI or visual elements, please include screenshots or GIFs here.
Ensure any sensitive data is redacted or generalized.
Further Comments / Questions
Any additional comments, questions, or areas where you'd like specific feedback.