Conversation
There was a problem hiding this comment.
Hello @chrisk314, 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!
Summary of Changes
This pull request focuses on enhancing the CI pipeline and pre-commit hooks for improved code quality and security. It introduces several new tools and configurations to achieve this, including radon and xenon for code quality checks, mypy for type analysis, and updates to Ruff for security-related linting. The author, chrisk314, has also included a TODO section for future improvements like test coverage checks and badges in the README.
Highlights
- Code Quality Checks: Introduces
radonandxenonas dev dependencies and adds axenoncheck to the lint and test workflow to enforce code complexity standards. - Pre-commit Hooks: Adds
mypyandxenonpre-commit hooks to automate type analysis and code complexity checks before committing. - Security Linting: Modifies Ruff's select rules to include the 'S' set, which contains security-related lint checks, enhancing the project's security posture.
- Dependency Management: Adds
pyzmqas a dependency and removes it from therayextra dependencies, and updates various dependency versions inpyproject.tomlanduv.lock.
Changelog
Click here to see the changelog
- .github/workflows/lint-test.yaml
- Adds a 'Code complexity' job that runs
xenonto check for code complexity issues, failing the CI if complexity exceeds configured thresholds. - Ensures all checks run even if some fail by using
if: always()formypy,xenon, andpytestjobs.
- Adds a 'Code complexity' job that runs
- .pre-commit-config.yaml
- Adds
mypypre-commit hook with additional dependencies for type checking. - Adds
xenonpre-commit hook to check code complexity before commits.
- Adds
- plugboard/connector/serde_channel.py
- Adds a
# noqa: S301comment to thepickle.loadscall in_deserialiseto suppress a security warning from Bandit, acknowledging the inherent risks of using pickle.
- Adds a
- plugboard/utils/random.py
- Adds a
# noqa: S311comment to therandom.choicescall ingen_rand_strto suppress a security warning from Bandit, acknowledging the use of a non-cryptographically secure random number generator.
- Adds a
- pyproject.toml
- Updates the project description to clarify the framework's capabilities.
- Adds
pyzmqas a direct dependency. - Moves
pyzmqfrom therayextra dependencies to the base dependencies. - Adds
radonandxenonto thedevdependencies. - Modifies Ruff's
selectrules to include security checks ('S'). - Adds a
radonsection to configure complexity and maintainability thresholds.
- uv.lock
- Updates various dependency versions and hashes to reflect changes in
pyproject.toml. - Adds entries for
mando,radon, andxenon. - Removes
pyzmqfromrayextra dependencies. - Updates platform markers for
click,ipykernel,portalocker, andtqdmto usesys_platforminstead ofplatform_system.
- Updates various dependency versions and hashes to reflect changes in
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
Did you know?
The 'noqa' comment in Python is used to tell linters (like Ruff or Flake8) to ignore a specific line of code. It's a combination of 'no' and 'quality assurance'.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request enhances the CI pipeline and pre-commit hooks with several code quality and security checks. The changes include adding radon and xenon for code quality, mypy for type analysis, and updating Ruff rules for security linting. Overall, this is a good step towards improving the project's code quality and security.
Summary of Findings
- Potential Security Risk with Pickle: The use of
pickle.loadswithout input validation inserde_channel.pyposes a security risk, as it can lead to arbitrary code execution if the received data is malicious. While anoqadirective is used to suppress the Bandit warning, it's crucial to address the underlying vulnerability with proper input sanitization or consider using a safer serialization method. - Insecure Random Number Generator: The use of
random.choicesinrandom.pyis flagged as potentially insecure by Bandit. While anoqadirective is used to suppress the warning, it's important to evaluate whether the usage context requires a cryptographically secure random number generator. If security is paramount, consider usingsecrets.choiceinstead. - uv.lock changes: The changes to
uv.lockare extensive and difficult to review manually. It's important to ensure that these changes are intentional and don't introduce any unintended dependency updates or conflicts. Consider using a tool to verify the integrity and correctness of the lockfile changes.
Merge Readiness
While the pull request introduces valuable code quality and security checks, the potential security risks associated with pickle.loads and random.choices need to be addressed. It's recommended to either implement proper input sanitization or use safer alternatives before merging. Additionally, the extensive changes in uv.lock should be carefully reviewed to ensure no unintended consequences. I am unable to approve this pull request, and other reviewers should carefully consider these points before merging.
97ea457 to
3498510
Compare
|
@toby-coleman this is ready for review. Hopefully the coverage bits are setup correctly... I'm not sure if the coverage svg url is correct. Maybe we need to do more setup of Github Pages first - not sure on that part... Unfortunately I'm not sure if we can verify the correctness until the pipeline has run on main, so might need a fixup post merge! |
toby-coleman
left a comment
There was a problem hiding this comment.
Looks good to me - one suggestion on the badges, and you'll need to merge from main to pick up the change I made to smoke test the package wheel file.
Co-authored-by: Toby Coleman <toby-coleman@users.noreply.github.com>
Added comments for sec ignores
# Summary Closes #96 by modifying the CI pipeline and pre-commit hooks with the enhancements listed below. Uses [this Github Action](https://github.com/marketplace/actions/coverage-badge) for the Coverage badge in the README. # Changes - Adds radon and xenon dev dependencies for code quality checks - Adds mypy and xenon pre-commit hooks for type analysis and code complexity checks respectively - Adds xenon check to lint and test github workflow for code complexity check - Modifies Ruff select rules to include the "S" set which contains security related lint checks - Adds test coverage check with pytest-cov plugin - Add badges to README for test coverage, licence, pypi version (anticpated), python versions, mypy typing status --------- Co-authored-by: Toby Coleman <toby-coleman@users.noreply.github.com>
Summary
Closes #96 by modifying the CI pipeline and pre-commit hooks with the enhancements listed below.
Uses this Github Action for the Coverage badge in the README.
Changes