-
Notifications
You must be signed in to change notification settings - Fork 13
Add Dependabot Cargo Vet workflow #2438
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
PR Reviewer Guide 🔍(Review updated until commit 5fe976d)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 5fe976d
Previous suggestionsSuggestions up to commit f21b728
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2438 +/- ##
==========================================
- Coverage 84.34% 81.90% -2.45%
==========================================
Files 141 141
Lines 10803 10803
==========================================
- Hits 9112 8848 -264
- Misses 1691 1955 +264 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…e diff handling and Codex integration
…for better risk assessment
8920b23 to
f4cea10
Compare
|
Persistent review updated to latest commit 5fe976d |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fe976d13b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| set -euo pipefail | ||
| python3 - <<'PY' | ||
| import os | ||
| path = os.environ["CONTEXT_FILE"] |
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.
This feels confusing, if the context file size will be capped, how can I know that something I add to the VETTING_POLICY is actually being used?
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.
I understand the concern — this is a trade-off we need to make.
There is no guarantee if the context is partially truncated, so the safer options are:
- Keep the context size conservative and within a fixed limit.
- Always try to include the full diff and the VETTING_POLICY, and if the context limit is reached, stop the automation instead of continuing with incomplete data.
- As a more complex alternative, split the diff into smaller parts and analyze them separately.
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.
Sorry, I don't think this trade off makes any sense. The CONTEXT_CHAR_LIMIT is limiting only the CONTEXT_FILE (VETTING_POLICY.md) size.
I cannot understand the rationale of possibly not including the full policy. If including the entire policy is not that important then it is not a policy. If instead it is important that the policy doesn't get too big, the action should fail if it does.
I can't think of a scenario where the policy is amended such that it has more than 90k characters, but we don't actually care if the entire updated policy is used. (what was the point of updating the policy in this scenario?)
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.
On this point, truncating the diff also seems non-sensical. We want to vet the crate upgrades because they could be malicious, truncating the diff is like saying to a possible supply chain attacker "you can include whatever you want as long as you pad it with enough characters". It'd be better to just outright fail the action if the diff size is too large or something. The 180000 character limit does seem too small either way, as the model being used can handle up to 400k tokens (and usually tokens are larger than 1 character).
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.
There's no capping of neither the policy nor the diff. If the context is too big for being checked in fact this should be treated as unvetted.
| continue | ||
| fi | ||
|
|
||
| { |
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.
why not initially write most of the file before the if then append the DIFF if it exsits? No need to repeat everything
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.
That’s fair. I duplicated the block to keep the “missing diff” case explicit, but I agree it’s cleaner to write the common header once and append the DIFF when it exists. I’ll change it.
| You are a Rust supply-chain security auditor. | ||
|
|
||
| Task: | ||
| - Assess ONLY the code changes shown in the provided diff for supply-chain/security risk. |
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.
isn't most of these in the VETTING_POLICY.md already?
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.
Good point — there was overlap. I originally split it per a model suggestion: put the role/policy in a separate file and keep only concise technical instructions (like the JSON-only output) in the workflow prompt. I did keep a little more in the workflow as extra guardrails, but I’ve now trimmed it to refer to VETTING_POLICY.md and keep only the minimal output requirements.
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.
I don't know what will work, if you find that it works better to repeat the instructions here, go ahead, I just want to understand it.
User description
Concise Summary
The intent of this PR is to automate cargo-vet for Dependabot crate bumps. It runs automatically on Dependabot PRs and can also be manually dispatched on any branch.
How It Works
Diagram Walkthrough
File Walkthrough
dependabot-auto-vet.yml
Create Dependabot cargo-vet workflow.github/workflows/dependabot-auto-vet.yml
VETTING_CONTEXT.md
Extend vetting context guidelinesVETTING_CONTEXT.md
cargo vet certifyPR Type
Enhancement
Description
Add Dependabot cargo-vet workflow
Remove obsolete VETTING_CONTEXT.md file
Add supply-chain/vet/VETTING_POLICY.md policy
Diagram Walkthrough
File Walkthrough
dependabot-auto-vet.yml
Add Dependabot cargo-vet workflow.github/workflows/dependabot-auto-vet.yml
VETTING_CONTEXT.md
Remove obsolete vetting contextVETTING_CONTEXT.md
VETTING_POLICY.md
Add vetting policy filesupply-chain/vet/VETTING_POLICY.md