Repository navigation
Conversation
Run pre-commit hooks (trailing-whitespace, end-of-file-fixer, check-yaml, codespell, etc.) in CI to enforce checks even when contributors skip local pre-commit. Skips rustfmt, clippy, and commit/push-time hooks that are already covered or not applicable. Signed-off-by: Chang Su <chang.s.su@oracle.com>
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 significantly enhances the project's continuous integration pipeline by introducing a dedicated job to run Highlights
Changelog
Ignored Files
Activity
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
|
📝 WalkthroughWalkthroughIntroduces a new pre-commit validation job in the CI workflow that runs Python checks before the existing lint suite, updates the finish job to depend on the new pre-commit job, and expands result gating to account for pre-commit and additional job failures. Minor formatting changes to proto files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces a pre-commit job to the CI pipeline to enforce code style and formatting checks automatically. The changes also include fixes for missing newlines at the end of two protobuf files (trtllm_service.proto and vllm_engine.proto), which were likely identified by the new pre-commit hooks. The changes are correct and improve code consistency. I have no specific comments as the changes are minor stylistic fixes and there are no issues of medium or higher severity.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pr-test-rust.yml (1)
576-584: 🧹 Nitpick | 🔵 TrivialLGTM!
The failure check correctly includes
pre-commit.resultalongside all other jobs from theneedsarray.💡 Optional: Consider handling `cancelled` job state
The current logic only checks for
"failure"but jobs can also be"cancelled"(e.g., due to timeouts or workflow cancellation). If you want stricter gating:- if [[ "${{ needs.pre-commit.result }}" == "failure" || \ + if [[ "${{ needs.pre-commit.result }}" != "success" && "${{ needs.pre-commit.result }}" != "skipped" ]] || \ + [[ "${{ needs.python-lint.result }}" != "success" && "${{ needs.python-lint.result }}" != "skipped" ]] || \This is pre-existing behavior, so addressing it is out of scope for this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pr-test-rust.yml around lines 576 - 584, The current GitHub Actions conditional in the if [[ ... ]] block only treats job results equal to "failure"; to also gate on cancelled jobs, update the condition that checks each needs.*.result (e.g., needs.pre-commit.result, needs.python-lint.result, needs.grpc-proto-build-check.result, etc.) to test for either "failure" OR "cancelled" (for example by combining checks like "${{ needs.X.result }}" == "failure" || "${{ needs.X.result }}" == "cancelled") so the overall if condition triggers when any dependent job is either failed or cancelled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/pr-test-rust.yml:
- Around line 576-584: The current GitHub Actions conditional in the if [[ ...
]] block only treats job results equal to "failure"; to also gate on cancelled
jobs, update the condition that checks each needs.*.result (e.g.,
needs.pre-commit.result, needs.python-lint.result,
needs.grpc-proto-build-check.result, etc.) to test for either "failure" OR
"cancelled" (for example by combining checks like "${{ needs.X.result }}" ==
"failure" || "${{ needs.X.result }}" == "cancelled") so the overall if condition
triggers when any dependent job is either failed or cancelled.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/pr-test-rust.ymlgrpc_client/proto/trtllm_service.protogrpc_client/proto/vllm_engine.proto
Description
Problem
Most pre-commit hooks (trailing-whitespace, end-of-file-fixer, check-yaml, check-toml, codespell, etc.) are only enforced locally. If a contributor skips or doesn't have pre-commit installed, these checks are bypassed — CI only runs ruff, rustfmt, and clippy.
Solution
Add a
pre-commitjob to the PR CI workflow that runspre-commit run --all-files. Hooks already covered by dedicated CI jobs (rustfmt, clippy) and commit/push-time hooks (no-commit-to-branch, branch-name-check, dco-check) are skipped via theSKIPenv var.Changes
pre-commitjob to.github/workflows/pr-test-rust.ymlpre-committo thefinishgate job (needs+ failure check)grpc_client/proto/trtllm_service.protoandgrpc_client/proto/vllm_engine.protoTest Plan
pre-commitjob will run on this PR itself, validating the setup.Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit