fix rules cache compat and script test deps#2960
fix rules cache compat and script test deps#2960akshat4703 wants to merge 2 commits intomandiant:masterfrom
Conversation
Summary of ChangesHello, 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 resolves two distinct issues that were hindering reliable local validation and potentially confusing contributors. It enhances the robustness of the rules caching mechanism by ensuring compatibility with schema changes and improves the stability of script tests by making their dependency expectations explicit, preventing failures due to missing optional packages. 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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses two separate issues: a cache compatibility bug and fragility in script tests with optional dependencies. The cache version is bumped to invalidate old caches, and the exception handling for cache loading is made more robust. The script tests now correctly skip tests for capa2sarif.py when its dependencies are not met. My review includes a couple of suggestions for improvement: in capa/rules/cache.py, I've suggested logging the specific exception when cache loading fails to aid in future debugging. In tests/test_scripts.py, I've proposed a change to improve code readability by breaking up a long line, following PEP 8 guidelines.
| except (AssertionError, EOFError, pickle.UnpicklingError, zlib.error, AttributeError, TypeError, ValueError): | ||
| logger.debug("rule set cache is invalid: %s", path) |
There was a problem hiding this comment.
While the broad exception handling is good for robustness, it would be beneficial for debugging to log the specific exception that was caught. This will help diagnose future cache corruption issues more easily.
| except (AssertionError, EOFError, pickle.UnpicklingError, zlib.error, AttributeError, TypeError, ValueError): | |
| logger.debug("rule set cache is invalid: %s", path) | |
| except (AssertionError, EOFError, pickle.UnpicklingError, zlib.error, AttributeError, TypeError, ValueError) as e: | |
| logger.debug("rule set cache is invalid: %s: %s", path, e) |
| logger = logging.getLogger(__name__) | ||
|
|
||
| CD = Path(__file__).resolve().parent | ||
| HAS_CAPA2SARIF_DEPS = importlib.util.find_spec("sarif_om") is not None and importlib.util.find_spec("jschema_to_python") is not None |
There was a problem hiding this comment.
This line is a bit long and combines two separate checks. For better readability and to adhere to standard line length limits (like PEP 8's recommendation), consider breaking it into multiple lines.
| HAS_CAPA2SARIF_DEPS = importlib.util.find_spec("sarif_om") is not None and importlib.util.find_spec("jschema_to_python") is not None | |
| HAS_SARIF_OM = importlib.util.find_spec("sarif_om") is not None | |
| HAS_JSCHEMA_TO_PYTHON = importlib.util.find_spec("jschema_to_python") is not None | |
| HAS_CAPA2SARIF_DEPS = HAS_SARIF_OM and HAS_JSCHEMA_TO_PYTHON |
References
- PEP 8, the style guide for Python code, recommends limiting lines to a maximum of 79 characters to improve readability. The current line exceeds this limit. (link)
1 participant
Summary
This PR addresses two unrelated but currently visible failures in one branch:
AttributeError: '_RuleFeatureIndex' object has no attribute 'bytes_prefix_index'_RuleFeatureIndexstructure changes.sarif_om,jschema_to_python).Why
These failures are pre-existing and not introduced by feature work, but they block reliable local validation and can confuse contributors.
Validation