chore: add verification workflow for AI policy enforcement #7976
Conversation
This PR adds technical enforcement for the AI usage policy: 1. Makefile target 'verify-with-proof': - Runs lint and tests - Uploads test logs to a GitHub Gist - Adds Tested-By and Test-Gist trailers to commit 2. GitHub Action using pull_request_target: - Uses author_association to distinguish trusted vs new contributors - Trusted (OWNER/MEMBER/COLLABORATOR): only requires Tested-By - New contributors: requires Tested-By + Test-Gist with valid Gist 3. Updated CONTRIBUTING_GUIDELINES.md: - Added Verification Workflow section with instructions Relates to jaegertracing#7878 Signed-off-by: Sapthagiri777 <sriramsapthagiri@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7976 +/- ##
=======================================
Coverage 95.48% 95.48%
=======================================
Files 316 316
Lines 16756 16756
=======================================
Hits 15999 15999
Misses 592 592
Partials 165 165
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:
|
There was a problem hiding this comment.
Pull request overview
This PR implements a technical enforcement mechanism for the AI usage policy introduced in PR #7932. It adds a verification workflow to ensure contributors run tests locally before submitting PRs, with stricter requirements for new contributors.
Changes:
- Adds
make verify-with-prooftarget that runs tests, uploads logs to GitHub Gist, and adds commit trailers - Implements GitHub Action workflow that validates
Tested-By:trailers for all contributors andTest-Gist:trailers for new contributors - Updates CONTRIBUTING_GUIDELINES.md with instructions on the verification workflow
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 18 comments.
| File | Description |
|---|---|
| Makefile | Adds verify-with-proof target to run tests, create Gist, and amend commit with trailers |
| .github/workflows/verify-pr.yml | New workflow using pull_request_target to verify commit trailers based on contributor trust level |
| CONTRIBUTING_GUIDELINES.md | Documents verification requirements and usage instructions for new workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/verify-pr.yml
Outdated
| # Trusted contributors: OWNER, MEMBER, COLLABORATOR | ||
| if [[ "$ASSOCIATION" == "OWNER" || "$ASSOCIATION" == "MEMBER" || "$ASSOCIATION" == "COLLABORATOR" ]]; then | ||
| echo "trusted=true" >> $GITHUB_OUTPUT | ||
| else | ||
| echo "trusted=false" >> $GITHUB_OUTPUT | ||
| fi |
There was a problem hiding this comment.
The documentation describes new contributors as "first-time, external" but doesn't mention what happens for contributors with CONTRIBUTOR association (those who have previously committed to the repo but aren't members/collaborators). According to the workflow logic on lines 31-35, CONTRIBUTOR would be treated as untrusted (not in the explicit trusted list), which means previous contributors who had commits merged would still need to provide Test-Gist proof.
This may be intentional, but it seems inconsistent with the spirit of trusting contributors who have proven themselves. Consider either:
- Adding
CONTRIBUTORto the trusted list on line 31 - Updating the documentation to clarify that even previous contributors need Test-Gist until they become members/collaborators
.github/workflows/verify-pr.yml
Outdated
| # Check if Gist description or content contains the commit hash | ||
| # The Gist description contains the commit hash from when it was created | ||
| GIST_ID=$(echo "$GIST_URL" | grep -oE '[a-f0-9]{32}' | head -1) | ||
| if [ -n "$GIST_ID" ]; then | ||
| GIST_INFO=$(curl -s "https://api.github.com/gists/$GIST_ID" 2>/dev/null || echo "") | ||
| GIST_DESC=$(echo "$GIST_INFO" | grep -o '"description":"[^"]*"' | head -1 | sed 's/"description":"//;s/"$//') | ||
| echo "Gist description: $GIST_DESC" | ||
| fi | ||
|
|
||
| echo "✅ Test-Gist trailer found and Gist is accessible" | ||
| echo "valid=true" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
The Gist validation logic extracts the Gist description and logs it, but never actually verifies that the commit hash in the description matches the current PR's commit hash (stored in COMMIT_SHA on line 60). This means the validation doesn't actually prove the Gist was created for this specific commit - a contributor could reuse an old Gist from a different commit.
After line 88 where GIST_DESC is extracted, you should add validation to check if GIST_DESC contains COMMIT_SHA, and set valid=false if it doesn't match.
.github/workflows/verify-pr.yml
Outdated
|
|
||
| # Check if Gist description or content contains the commit hash | ||
| # The Gist description contains the commit hash from when it was created | ||
| GIST_ID=$(echo "$GIST_URL" | grep -oE '[a-f0-9]{32}' | head -1) |
There was a problem hiding this comment.
The regex pattern [a-f0-9]{32} is used to extract the Gist ID, but GitHub Gist IDs are actually 32 hexadecimal characters long. However, the pattern only matches exactly 32 characters, and the URL might contain other hex sequences. More importantly, GitHub Gist IDs can be either 20 or 32 characters depending on when they were created. The current pattern might miss shorter IDs or incorrectly extract partial hashes from the URL path.
Consider using a more robust pattern that matches the actual Gist ID from the URL structure, such as extracting from the path after gist.github.com/ or gist.github.com/username/.
| GIST_ID=$(echo "$GIST_URL" | grep -oE '[a-f0-9]{32}' | head -1) | |
| GIST_ID=$(echo "$GIST_URL" | grep -oE 'gist.github.com/([^/]+/)?[0-9a-f]{20,32}' | sed -E 's#.*/##' | head -1) |
Makefile
Outdated
| @COMMIT=$$(git log -1 --pretty=%H); \ | ||
| GIST_URL=$$(gh gist create test.log -d "Test logs for Jaeger commit $$COMMIT" --public 2>/dev/null); \ | ||
| if [ -z "$$GIST_URL" ]; then \ | ||
| echo "❌ Failed to create Gist. Make sure 'gh' CLI is installed and authenticated."; \ | ||
| exit 1; \ | ||
| fi; \ | ||
| MSG=$$(git log -1 --pretty=%B); \ | ||
| NAME=$$(git config user.name); \ | ||
| EMAIL=$$(git config user.email); \ | ||
| git commit --amend --no-edit -m "$$MSG" -m "Tested-By: $$NAME <$$EMAIL>" -m "Test-Gist: $$GIST_URL"; \ |
There was a problem hiding this comment.
The Makefile target doesn't verify that there's actually a commit to amend. If a user runs this command when there are no commits in the repository, or in a detached HEAD state, the git commit --amend command will fail. This could happen in automated environments or unusual git states.
Consider adding a check before line 264 to verify that HEAD points to a valid commit, for example: git rev-parse --verify HEAD >/dev/null 2>&1 || { echo "No commit to amend"; exit 1; }
| Before pushing your PR, run: | ||
| ```bash | ||
| make verify-with-proof | ||
| ``` | ||
|
|
||
| This command will: | ||
| 1. Run lint and tests locally. | ||
| 2. Upload your test logs to a GitHub Gist. | ||
| 3. Add `Tested-By:` and `Test-Gist:` trailers to your commit. | ||
|
|
||
| Then push with: | ||
| ```bash | ||
| git push --force-with-lease | ||
| ``` |
There was a problem hiding this comment.
If a contributor runs make verify-with-proof to get the trailers, then makes additional commits, the Test-Gist will reference an old commit hash that doesn't match the PR head. The workflow checks for a Gist and validates it's accessible, but as noted in another comment, it doesn't verify the commit hash in the Gist description matches the PR's current commit.
This creates a loophole where contributors could:
- Run verify-with-proof on a simple/empty change
- Get the Tested-By and Test-Gist trailers
- Amend or add commits with actual changes
- Push, and the workflow would pass
The documentation (line 156) instructs users to push with --force-with-lease after running the command, but doesn't warn against making additional commits afterward. Consider adding a warning in the documentation about this limitation.
Makefile
Outdated
| # Verify PR with proof for AI policy compliance | ||
| # This target runs lint and tests, uploads logs to a Gist, and adds trailers to the commit. | ||
| # Required for new contributors to prove tests were actually run locally. | ||
| .PHONY: verify-with-proof | ||
| verify-with-proof: lint test | ||
| @echo "✅ Lint and tests passed. Uploading proof to Gist..." | ||
| @$(MAKE) test > test.log 2>&1 || true |
There was a problem hiding this comment.
The test target is executed twice: once as a dependency on line 261 and again on line 263. This is wasteful and could cause confusion. Additionally, the || true on line 263 suppresses test failures, which means the test.log file could contain failed test output, but the workflow would still proceed to create a Gist and amend the commit. This defeats the purpose of proving tests passed.
Consider removing line 263 entirely and capturing the test output from the dependency run instead, or redirect output during the dependency execution.
| # Verify PR with proof for AI policy compliance | |
| # This target runs lint and tests, uploads logs to a Gist, and adds trailers to the commit. | |
| # Required for new contributors to prove tests were actually run locally. | |
| .PHONY: verify-with-proof | |
| verify-with-proof: lint test | |
| @echo "✅ Lint and tests passed. Uploading proof to Gist..." | |
| @$(MAKE) test > test.log 2>&1 || true | |
| .PHONY: test-with-log | |
| test-with-log: | |
| $(MAKE) test > test.log 2>&1 | |
| # Verify PR with proof for AI policy compliance | |
| # This target runs lint and tests, uploads logs to a Gist, and adds trailers to the commit. | |
| # Required for new contributors to prove tests were actually run locally. | |
| .PHONY: verify-with-proof | |
| verify-with-proof: lint test-with-log | |
| @echo "✅ Lint and tests passed. Uploading proof to Gist..." |
Makefile
Outdated
| @echo "✅ Lint and tests passed. Uploading proof to Gist..." | ||
| @$(MAKE) test > test.log 2>&1 || true | ||
| @COMMIT=$$(git log -1 --pretty=%H); \ | ||
| GIST_URL=$$(gh gist create test.log -d "Test logs for Jaeger commit $$COMMIT" --public 2>/dev/null); \ |
There was a problem hiding this comment.
The GitHub CLI command gh gist create with 2>/dev/null suppresses all stderr output, which means if the command fails for reasons other than not being installed/authenticated (such as network issues, API rate limits, or temporary GitHub outages), the error message will be hidden and the user will only see the generic "Failed to create Gist" message.
Consider removing 2>/dev/null or capturing stderr to provide more helpful error messages to users when Gist creation fails.
| GIST_URL=$$(gh gist create test.log -d "Test logs for Jaeger commit $$COMMIT" --public 2>/dev/null); \ | |
| GIST_URL=$$(gh gist create test.log -d "Test logs for Jaeger commit $$COMMIT" --public); \ |
.github/workflows/verify-pr.yml
Outdated
| if [ -z "$GIST_URL" ]; then | ||
| echo "❌ Missing Test-Gist trailer. New contributors must run 'make verify-with-proof' before pushing." | ||
| echo "valid=false" >> $GITHUB_OUTPUT | ||
| exit 0 | ||
| fi | ||
|
|
||
| echo "Found Test-Gist URL: $GIST_URL" | ||
|
|
||
| # Fetch the Gist and check for commit hash | ||
| GIST_CONTENT=$(curl -s "$GIST_URL/raw" 2>/dev/null || echo "") | ||
|
|
||
| if [ -z "$GIST_CONTENT" ]; then | ||
| echo "❌ Could not fetch Gist content from: $GIST_URL" | ||
| echo "valid=false" >> $GITHUB_OUTPUT | ||
| exit 0 |
There was a problem hiding this comment.
The Test-Gist validation step uses exit 0 when validation fails (lines 68, 79), which means the step will succeed even when the Gist is invalid or missing. The actual failure check happens later in the "Verify results" step by checking if valid=true. However, this creates a less clear flow where a step that finds a problem still exits successfully.
While this works, it would be clearer to either: (1) fail the step immediately and use continue-on-error: true with appropriate output handling, or (2) add a comment explaining why exit 0 is used here (to allow the final verification step to provide a consolidated error message).
Makefile
Outdated
| @$(MAKE) test > test.log 2>&1 || true | ||
| @COMMIT=$$(git log -1 --pretty=%H); \ | ||
| GIST_URL=$$(gh gist create test.log -d "Test logs for Jaeger commit $$COMMIT" --public 2>/dev/null); \ | ||
| if [ -z "$$GIST_URL" ]; then \ | ||
| echo "❌ Failed to create Gist. Make sure 'gh' CLI is installed and authenticated."; \ | ||
| exit 1; \ | ||
| fi; \ | ||
| MSG=$$(git log -1 --pretty=%B); \ | ||
| NAME=$$(git config user.name); \ | ||
| EMAIL=$$(git config user.email); \ | ||
| git commit --amend --no-edit -m "$$MSG" -m "Tested-By: $$NAME <$$EMAIL>" -m "Test-Gist: $$GIST_URL"; \ | ||
| echo "✅ Commit amended with Tested-By and Test-Gist trailers."; \ | ||
| echo " Gist URL: $$GIST_URL"; \ | ||
| echo ""; \ | ||
| echo "Now run: git push --force-with-lease" |
There was a problem hiding this comment.
The test.log file created on line 263 is never cleaned up. This file will remain in the working directory after the command completes, which could cause confusion or clutter in the contributor's workspace. It might also accidentally be committed if the contributor runs git add . or similar commands.
Consider either:
- Adding a cleanup step at the end to remove test.log
- Creating the file in a temporary directory (e.g.,
/tmp/test-$$.log) - Adding test.log to .gitignore if it's not already there
|
Hey @jkowall @yurishkuro 👋 Thanks for taking a look! Copilot flagged several issues - I've reviewed them and here's my plan: High priority (will fix):
Medium priority: I can push a fix addressing all of these, or start with a smaller subset if you'd prefer to iterate. Let me know what you think! |
Addresses Copilot review comments: Makefile changes: - Add separate test-with-log target (tests run once, not twice) - Remove || true that masked test failures - Add gh CLI availability check before running - Add HEAD safety check before git commit --amend - Store commit SHA in Gist for verification - Clean up test.log after creating Gist - Use git commit --trailer for safer message handling GitHub Action changes: - Add permissions block for pull_request_target security - Verify Gist commit SHA matches PR head SHA - Fix Gist ID regex to handle 20-32 character IDs - Add explicit output for trusted contributors - Document that CONTRIBUTOR association still requires Test-Gist Documentation changes: - Move gh CLI prerequisite to top of section - Add warning about re-running verify-with-proof after code changes Relates to jaegertracing#7878 Signed-off-by: Sapthagiri777 <sriramsapthagiri@gmail.com>
|
@jkowall @yurishkuro
Let me know if anything else needs . |
|
Hey @yurishkuro @jkowall |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Checkout PR head | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| ref: ${{ github.event.pull_request.head.sha }} | ||
| fetch-depth: 1 |
There was a problem hiding this comment.
This repo appears to pin GitHub Actions to full commit SHAs (e.g., actions/checkout@... # v6.0.0) in other workflows. Here actions/checkout@v4 is unpinned, which weakens supply-chain security and is inconsistent with the established pattern. Pin actions/checkout to a specific commit SHA (with an inline version comment) like the other workflows.
| jobs: | ||
| verify: | ||
| runs-on: ubuntu-latest | ||
| steps: |
There was a problem hiding this comment.
Other workflows in this repo commonly include step-security/harden-runner to audit/block egress, and this workflow performs outbound network calls (curl to gists / api.github.com). Adding harden-runner here would align with the repo’s security posture and reduce risk, especially since this runs on pull_request_target.
| steps: | |
| steps: | |
| - name: Harden GitHub Runner | |
| uses: step-security/harden-runner@v2 | |
| with: | |
| egress-policy: audit |
CONTRIBUTING_GUIDELINES.md
Outdated
| **For new contributors (first-time, external):** | ||
| - You must also include a `Test-Gist:` trailer with a link to your test logs. | ||
| - The Gist contains your commit SHA, proving the tests were run on that specific commit. | ||
| - This proves you actually ran tests, not just copied a line. |
There was a problem hiding this comment.
The documentation says the Test-Gist requirement is only for "new contributors (first-time, external)", but the workflow treats any non-OWNER/MEMBER/COLLABORATOR association (including CONTRIBUTOR) as untrusted and requires Test-Gist. Update the docs to match the actual enforcement (or adjust the workflow’s trust rules to match the documented policy).
CONTRIBUTING_GUIDELINES.md
Outdated
| git push --force-with-lease | ||
| ``` | ||
|
|
||
| **Important:** If you modify your code after running `make verify-with-proof`, you must run it again. The CI verifies that the Gist's commit SHA matches your PR head. |
There was a problem hiding this comment.
This states that CI verifies the Gist’s commit SHA matches the PR head, but the workflow currently allows the PR to proceed if it cannot extract a SHA from the Gist content/description (it logs a warning and passes). Either tighten the workflow to fail when the SHA can’t be verified, or soften this wording so contributors aren’t told there’s a strict SHA match when there isn’t.
| **Important:** If you modify your code after running `make verify-with-proof`, you must run it again. The CI verifies that the Gist's commit SHA matches your PR head. | |
| **Important:** If you modify your code after running `make verify-with-proof`, you must run it again. The CI will attempt to verify that the Gist's commit SHA matches your PR head when possible. |
Makefile
Outdated
| @COMMIT=$$(git rev-parse HEAD); \ | ||
| echo "Commit SHA: $$COMMIT" > test.log.tmp; \ | ||
| echo "---" >> test.log.tmp; \ | ||
| cat test.log >> test.log.tmp; \ | ||
| mv test.log.tmp test.log; \ | ||
| GIST_URL=$$(gh gist create test.log -d "Test logs for Jaeger commit $$COMMIT" --public); \ |
There was a problem hiding this comment.
verify-with-proof captures the current HEAD SHA into the log and Gist before amending the commit to add the trailers. Because git commit --amend changes the commit message, it will always create a new commit SHA, so the Gist SHA will not match the PR head SHA and the verify-pr.yml check will fail for new contributors.
Adjust the approach so the proof is tied to something that doesn’t change when adding trailers (e.g., commit tree SHA), or update the Gist after the amend so it contains the final amended commit SHA, or change the workflow comparison accordingly.
Makefile
Outdated
| GIST_URL=$$(gh gist create test.log -d "Test logs for Jaeger commit $$COMMIT" --public); \ | ||
| if [ -z "$$GIST_URL" ]; then \ | ||
| echo "❌ Failed to create Gist. Make sure 'gh' CLI is authenticated."; \ | ||
| rm -f test.log; \ |
There was a problem hiding this comment.
On Gist creation failure, the recipe deletes test.log (rm -f test.log), which removes the only local artifact that would help the contributor troubleshoot or retry uploading. Consider keeping the file on failure (and only cleaning it up on success) so contributors can inspect or manually upload the logs.
| rm -f test.log; \ |
.github/workflows/verify-pr.yml
Outdated
| # Extract Test-Gist URL | ||
| GIST_URL=$(echo "$COMMIT_MSG" | grep "Test-Gist:" | awk '{print $2}' | head -1) | ||
|
|
||
| if [ -z "$GIST_URL" ]; then | ||
| echo "❌ Missing Test-Gist trailer. New contributors must run 'make verify-with-proof' before pushing." | ||
| echo "valid=false" >> $GITHUB_OUTPUT | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "Found Test-Gist URL: $GIST_URL" | ||
|
|
||
| # Extract Gist ID from URL (handles both 20 and 32 character IDs) | ||
| GIST_ID=$(echo "$GIST_URL" | grep -oE '[0-9a-f]{20,32}' | tail -1) | ||
|
|
||
| if [ -z "$GIST_ID" ]; then | ||
| echo "❌ Could not extract Gist ID from URL: $GIST_URL" | ||
| echo "valid=false" >> $GITHUB_OUTPUT | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "Gist ID: $GIST_ID" | ||
|
|
||
| # Fetch Gist content | ||
| GIST_CONTENT=$(curl -s "$GIST_URL/raw" 2>/dev/null || echo "") | ||
|
|
There was a problem hiding this comment.
GIST_URL is taken directly from the commit message and then used in curl -s "$GIST_URL/raw". This allows an untrusted PR author to make the workflow curl an arbitrary URL (SSRF), which is especially risky under pull_request_target. Restrict accepted URLs to https://gist.github.com/<user>/<id> (or https://gist.githubusercontent.com/...) and fetch content via the GitHub Gist API using the extracted GIST_ID (building the URL yourself) instead of curling a user-provided URL.
Fixes based on Copilot review feedback: Critical bug fix: - Use tree SHA instead of commit SHA in verification - Tree SHA represents code content and doesn't change when amending - This fixes the issue where SHA was captured before git commit --amend Security improvements: - Pin actions/checkout to full SHA (11bd71901bbe5b1630ceea73d27597364c9af683) - Add step-security/harden-runner for egress auditing - Fetch Gist via GitHub API using extracted ID (prevents SSRF) - Keep test.log on Gist creation failure for debugging Documentation: - Change 'first-time, external' to 'non-trusted contributors' - Update wording to 'CI will attempt to verify' (softer language) - Change 'commit SHA' to 'tree SHA' to match implementation Relates to jaegertracing#7878 Signed-off-by: Sapthagiri777 <sriramsapthagiri@gmail.com>
|
Hey @jkowall @yurishkuro 👋 Pushed fixes for the latest Copilot feedback: Critical bug fix:
Security improvements:
Documentation:
Note: Codecov failure is unrelated - it shows "all modified lines are covered" but has fewer uploads than base. This PR only adds workflow/docs files. Let me know if anything else needs tweaking! |
|
Hey @jkowall @yurishkuro ,Pushed fixes for the latest Copilot feedback. All checks are now passing (60/60). Changes in this update:
Ready for review when you have time. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Makefile
Outdated
| @$(MAKE) test 2>&1 | tee test.log; \ | ||
| EXIT_CODE=$${PIPESTATUS[0]}; \ | ||
| if [ $$EXIT_CODE -ne 0 ]; then \ | ||
| echo "❌ Tests failed. See test.log for details."; \ | ||
| exit $$EXIT_CODE; \ | ||
| fi; \ |
There was a problem hiding this comment.
test-with-log only checks PIPESTATUS[0] (exit of $(MAKE) test) and will still report success even if tee fails (e.g., disk full / permission issues), which means you may upload an incomplete/missing log. Consider using set -o pipefail for the pipeline and/or explicitly checking tee’s exit status so log capture failures fail the target.
| @$(MAKE) test 2>&1 | tee test.log; \ | |
| EXIT_CODE=$${PIPESTATUS[0]}; \ | |
| if [ $$EXIT_CODE -ne 0 ]; then \ | |
| echo "❌ Tests failed. See test.log for details."; \ | |
| exit $$EXIT_CODE; \ | |
| fi; \ | |
| @set -o pipefail; \ | |
| $(MAKE) test 2>&1 | tee test.log; \ | |
| TEST_EXIT_CODE=$${PIPESTATUS[0]}; \ | |
| TEE_EXIT_CODE=$${PIPESTATUS[1]}; \ | |
| if [ $$TEST_EXIT_CODE -ne 0 ]; then \ | |
| echo "❌ Tests failed. See test.log for details."; \ | |
| exit $$TEST_EXIT_CODE; \ | |
| fi; \ | |
| if [ $$TEE_EXIT_CODE -ne 0 ]; then \ | |
| echo "❌ Log capture failed (tee exited with $$TEE_EXIT_CODE). Test results may not be fully recorded."; \ | |
| exit $$TEE_EXIT_CODE; \ | |
| fi; \ |
Makefile
Outdated
| @echo "✅ Lint and tests passed. Uploading proof to Gist..." | ||
| @# Use tree SHA - it represents the code content and doesn't change when amending commit message | ||
| @TREE_SHA=$$(git rev-parse HEAD^{tree}); \ | ||
| echo "Tree SHA: $$TREE_SHA" > test.log.tmp; \ | ||
| echo "---" >> test.log.tmp; \ | ||
| cat test.log >> test.log.tmp; \ | ||
| mv test.log.tmp test.log; \ | ||
| GIST_URL=$$(gh gist create test.log -d "Test logs for Jaeger tree $$TREE_SHA" --public); \ | ||
| if [ -z "$$GIST_URL" ]; then \ | ||
| echo "❌ Failed to create Gist. Make sure 'gh' CLI is authenticated."; \ | ||
| echo " test.log kept for manual inspection."; \ | ||
| exit 1; \ | ||
| fi; \ | ||
| NAME=$$(git config user.name); \ | ||
| EMAIL=$$(git config user.email); \ | ||
| git commit --amend --no-edit \ | ||
| --trailer "Tested-By: $$NAME <$$EMAIL>" \ | ||
| --trailer "Test-Gist: $$GIST_URL"; \ | ||
| rm -f test.log; \ | ||
| echo "✅ Commit amended with Tested-By and Test-Gist trailers."; \ | ||
| echo " Gist URL: $$GIST_URL"; \ | ||
| echo ""; \ |
There was a problem hiding this comment.
verify-with-proof runs a long \-continued shell script without set -e / error checks. If gh gist create or git commit --amend fails (e.g., auth/signing hooks), the recipe can continue, delete test.log, and print the success message anyway. Consider enabling set -euo pipefail for this block (or checking exit codes) and only removing test.log / printing success after the amend succeeds.
CONTRIBUTING_GUIDELINES.md
Outdated
| **For non-trusted contributors (not OWNER/MEMBER/COLLABORATOR):** | ||
| - You must also include a `Test-Gist:` trailer with a link to your test logs. | ||
| - The Gist contains your commit SHA, proving the tests were run on that specific commit. | ||
| - This proves you actually ran tests, not just copied a line. | ||
|
|
||
| **How to do this:** | ||
|
|
||
| Before pushing your PR, run: | ||
| ```bash | ||
| make verify-with-proof | ||
| ``` | ||
|
|
||
| This command will: | ||
| 1. Run lint and tests locally (fails if tests fail). | ||
| 2. Upload your test logs to a GitHub Gist (includes commit SHA). | ||
| 3. Add `Tested-By:` and `Test-Gist:` trailers to your commit. |
There was a problem hiding this comment.
This section says the Gist contains the commit SHA, but both make verify-with-proof and the CI verifier use the tree SHA (HEAD^{tree}) for validation. Please update the wording (and the step list) to consistently refer to tree SHA, otherwise contributors will produce the wrong proof format.
| - name: Harden GitHub Runner | ||
| uses: step-security/harden-runner@v2 | ||
| with: | ||
| egress-policy: audit |
There was a problem hiding this comment.
Other workflows pin step-security/harden-runner to an immutable commit SHA (e.g., .github/workflows/ci-unit-tests.yml:25), but this workflow uses the floating @v2 tag. For supply-chain safety and consistency, pin step-security/harden-runner to a commit SHA here as well.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: SRIRAM SAPTHAGIRI <sriramsapthagiri@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/verify-pr.yml
Outdated
| # Fetch Gist content securely using GitHub API (not user-provided URL) | ||
| GIST_CONTENT=$(curl -s "https://api.github.com/gists/$GIST_ID" | jq -r '.files | to_entries[0].value.content // empty') | ||
|
|
There was a problem hiding this comment.
This workflow calls the GitHub API without authentication. That is susceptible to low unauthenticated rate limits and can cause flaky verification failures. Please pass Authorization: Bearer ${{ github.token }} (or set GITHUB_TOKEN and use it) when calling the API.
.github/workflows/verify-pr.yml
Outdated
| echo "$COMMIT_MSG" | ||
| echo "---" | ||
|
|
||
| if echo "$COMMIT_MSG" | grep -q "Tested-By:"; then |
There was a problem hiding this comment.
Trailer detection/parsing is currently done via substring grep on the whole commit message. This can be fooled by mentioning Tested-By: / Test-Gist: in the body and may mis-parse values. Consider parsing trailers via git interpret-trailers --parse (or at least matching ^Tested-By: / ^Test-Gist: and extracting the value from the parsed trailer block) to align with the documented requirement of commit trailers.
| if echo "$COMMIT_MSG" | grep -q "Tested-By:"; then | |
| TRAILERS=$(printf '%s\n' "$COMMIT_MSG" | git interpret-trailers --parse) | |
| echo "Parsed trailers:" | |
| echo "$TRAILERS" | |
| echo "---" | |
| if echo "$TRAILERS" | grep -qE '^Tested-By:[[:space:]]*'; then |
Makefile
Outdated
| @TREE_SHA=$$(git rev-parse HEAD^{tree}); \ | ||
| echo "Tree SHA: $$TREE_SHA" > test.log.tmp; \ | ||
| echo "---" >> test.log.tmp; \ | ||
| cat test.log >> test.log.tmp; \ | ||
| mv test.log.tmp test.log; \ | ||
| GIST_URL=$$(gh gist create test.log -d "Test logs for Jaeger tree $$TREE_SHA" --public); \ | ||
| if [ -z "$$GIST_URL" ]; then \ | ||
| echo "❌ Failed to create Gist. Make sure 'gh' CLI is authenticated."; \ | ||
| echo " test.log kept for manual inspection."; \ | ||
| exit 1; \ | ||
| fi; \ | ||
| NAME=$$(git config user.name); \ | ||
| EMAIL=$$(git config user.email); \ | ||
| git commit --amend --no-edit \ | ||
| --trailer "Tested-By: $$NAME <$$EMAIL>" \ | ||
| --trailer "Test-Gist: $$GIST_URL"; \ | ||
| rm -f test.log; \ |
There was a problem hiding this comment.
The long shell block in verify-with-proof doesn’t enable set -e (and uses ; separators), so a failure in gh gist create (after the explicit check), git commit --amend, or rm can be masked by subsequent echo commands, potentially making the target succeed even though the commit wasn’t amended. Please add set -e (and ideally set -o pipefail) at the start of the block or use && chaining/check exit codes so failures propagate correctly.
Makefile
Outdated
| exit 1; \ | ||
| fi; \ | ||
| NAME=$$(git config user.name); \ | ||
| EMAIL=$$(git config user.email); \ |
There was a problem hiding this comment.
Tested-By is populated from git config user.name / user.email without validation. If either value is unset/empty, this will amend the commit with an invalid trailer (e.g., Tested-By: <>). Please add checks that both are non-empty (and ideally that the email looks like an email) before amending.
| EMAIL=$$(git config user.email); \ | |
| EMAIL=$$(git config user.email); \ | |
| if [ -z "$$NAME" ]; then \ | |
| echo "❌ git config user.name is not set or empty. Please configure it before running 'make verify-with-proof'."; \ | |
| exit 1; \ | |
| fi; \ | |
| if [ -z "$$EMAIL" ]; then \ | |
| echo "❌ git config user.email is not set or empty. Please configure it before running 'make verify-with-proof'."; \ | |
| exit 1; \ | |
| fi; \ | |
| if ! echo "$$EMAIL" | grep -Eq '^[^@[:space:]]+@[^@[:space:]]+\.[^@[:space:]]+'; then \ | |
| echo "❌ git config user.email ('$$EMAIL') does not look like a valid email address."; \ | |
| echo " Please set a valid email before running 'make verify-with-proof'."; \ | |
| exit 1; \ | |
| fi; \ |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CONTRIBUTING_GUIDELINES.md
Outdated
|
|
||
| **Prerequisites:** | ||
| - [GitHub CLI (`gh`)](https://cli.github.com/) installed and authenticated | ||
| - At least one commit on your branch |
There was a problem hiding this comment.
The proof is based on the commit’s tree SHA, but the doc only lists “at least one commit” as a prerequisite. Since uncommitted changes won’t be represented by the tree SHA (and can cause CI verification failures/confusion), consider documenting that contributors should commit all changes / have a clean working tree before running make verify-with-proof (or update this if the Makefile is changed to enforce it).
| - At least one commit on your branch | |
| - At least one commit on your branch | |
| - A clean working tree (all changes you want verified are staged and committed) |
Makefile
Outdated
| @# Check that there's a commit to amend | ||
| @if ! git rev-parse --verify HEAD >/dev/null 2>&1; then \ | ||
| echo "❌ No commit to amend. Please commit your changes first, then run 'make verify-with-proof'."; \ | ||
| exit 1; \ | ||
| fi | ||
| @echo "✅ Lint and tests passed. Uploading proof to Gist..." | ||
| @# Use tree SHA - it represents the code content and doesn't change when amending commit message | ||
| @set -euo pipefail; TREE_SHA=$$(git rev-parse HEAD^{tree}); \ | ||
| echo "Tree SHA: $$TREE_SHA" > test.log.tmp; \ |
There was a problem hiding this comment.
verify-with-proof runs tests against the working tree, but the proof SHA is computed from HEAD^{tree}. If the working tree (or index) has uncommitted changes, the uploaded log can correspond to different code than the tree SHA, and CI validation can fail unexpectedly. Consider failing early unless git status is clean (or explicitly deriving the tree SHA from the exact content being tested).
Makefile
Outdated
| exit 1; \ | ||
| fi; \ | ||
| NAME=$$(git config user.name); \ | ||
| EMAIL=$$(git config user.email); \ |
There was a problem hiding this comment.
Tested-By trailer is constructed from git config user.name / user.email, but these can be empty (common in fresh environments), producing an invalid trailer like Tested-By: <>. Add a check that both values are non-empty (and fail with an actionable message) before amending the commit.
| EMAIL=$$(git config user.email); \ | |
| EMAIL=$$(git config user.email); \ | |
| if [ -z "$$NAME" ] || [ -z "$$EMAIL" ]; then \ | |
| echo "❌ Git user.name and/or user.email are not set."; \ | |
| echo " Please configure your git identity, for example:"; \ | |
| echo " git config --global user.name \"Your Name\""; \ | |
| echo " git config --global user.email \"you@example.com\""; \ | |
| echo " Then re-run: make verify-with-proof"; \ | |
| exit 1; \ | |
| fi; \ |
.github/workflows/verify-pr.yml
Outdated
| # Fetch Gist content securely using GitHub API (not user-provided URL) | ||
| GIST_CONTENT=$(curl -s "https://api.github.com/gists/$GIST_ID" | jq -r '.files | to_entries[0].value.content // empty') | ||
|
|
There was a problem hiding this comment.
The Gist fetch uses an unauthenticated curl and does not fail on non-2xx responses. This is prone to GitHub API rate limiting and can silently return an error JSON that then looks like “missing content”. Pass Authorization: Bearer ${{ github.token }} (export via env:) and use curl --fail-with-body (or similar) so HTTP errors are handled explicitly.
|
Hey @jkowall @yurishkuro 👋 Just following up - Copilot continues to suggest more improvements with each review cycle (clean working tree checks, git config validation, API authentication, etc.). While these are valid suggestions, the core functionality is complete and working:
Would you be able to review the PR and let me know which changes (if any) are required for merge? I want to make sure I'm focusing on what's actually needed rather than chasing an endless stream of suggestions. Thank you! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/verify-pr.yml
Outdated
| # Fetch Gist content securely using GitHub API (not user-provided URL) | ||
| GIST_CONTENT=$(curl -s "https://api.github.com/gists/$GIST_ID" | jq -r '.files | to_entries[0].value.content // empty') | ||
|
|
||
| if [ -z "$GIST_CONTENT" ]; then | ||
| echo "❌ Could not fetch Gist content for ID: $GIST_ID" | ||
| echo "valid=false" >> $GITHUB_OUTPUT | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
jq -r '.files | to_entries[0].value.content' is brittle: object entry order isn’t guaranteed and a multi-file Gist could make this read the wrong file. It can also hard-fail with a jq error (before your [ -z "$GIST_CONTENT" ] check) if .files is missing (e.g., 404/abuse-limit responses). Prefer selecting the expected filename (e.g., test.log) and handling non-200 responses explicitly (e.g., curl -f + status check, or jq with ?/// empty).
yurishkuro
left a comment
yurishkuro
left a comment
There was a problem hiding this comment.
what is your plan for testing this? Because it's using pull_request_target trigger it won't run on your own PR.
Makefile
Outdated
| # Fails if tests fail or if tee fails (checks both PIPESTATUS values) | ||
| .PHONY: test-with-log | ||
| test-with-log: | ||
| @set -o pipefail; \ |
There was a problem hiding this comment.
Normal make execution is to stop if any statement fails, which is what you need here. The whole thing can be one line:
test-with-log:
set -o pipefail; $(MAKE) test 2>&1 | tee .test.log
Also use a dot file .test.log and add it to .gitignore, there's no need to pollute main directory.
| # - GitHub CLI (gh) installed and authenticated: https://cli.github.com/ | ||
| # - At least one commit on your branch | ||
| .PHONY: verify-with-proof | ||
| verify-with-proof: lint test-with-log |
There was a problem hiding this comment.
please move to a script instead of doing in Makefile
There was a problem hiding this comment.
make sure to run linter for scripts
CONTRIBUTING_GUIDELINES.md
Outdated
| - [GitHub CLI (`gh`)](https://cli.github.com/) installed and authenticated | ||
| - At least one commit on your branch | ||
|
|
||
| **For all contributors:** |
There was a problem hiding this comment.
we do not have to spell out how the validation is done (it just makes it easier for LLM to cheat)
.github/workflows/verify-pr.yml
Outdated
| - name: Check for Test-Gist trailer (new contributors only) | ||
| id: test-gist | ||
| if: steps.author.outputs.trusted == 'false' | ||
| run: | |
There was a problem hiding this comment.
Move all this logic to a script where it can be easily tested. You can even write unit tests for the script (see scripts/utils/compute-tags.test.sh).
.github/workflows/verify-pr.yml
Outdated
| name: Verify PR | ||
|
|
||
| on: | ||
| pull_request_target: |
There was a problem hiding this comment.
Why do you need pull_request_target instead of just pull_request?
|
Hi @yurishkuro, thanks for the review! Some of the complexity (PIPESTATUS checks, detailed error handling) came from Copilot suggestions. Happy to simplify as you suggested. I'll refactor to:
For |
All PRs are from forks. |
github-actions
bot
added
the
waiting-for-author
Changes: - Simplify test-with-log to one line with .test.log - Move verify-with-proof logic to scripts/verify-with-proof.sh - Move workflow logic to scripts/verify-pr.sh - Change pull_request_target to pull_request - Remove detailed validation docs (prevents LLM cheating) - Add .test.log to .gitignore - All scripts pass shellcheck Relates to jaegertracing#7878 Signed-off-by: Sapthagiri777 <sriramsapthagiri@gmail.com>
github-actions
bot
removed
the
waiting-for-author
|
@yurishkuro Refactored as suggested:
Ready for review! |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| on: | ||
| pull_request: | ||
| types: [opened, synchronize, reopened] |
There was a problem hiding this comment.
Critical workflow event mismatch: The workflow uses pull_request but the PR description explicitly states it should use pull_request_target to work with PRs from forks. With pull_request, the workflow runs in the context of the fork with limited permissions and may not have access to check out code properly or access secrets. This breaks the stated functionality.
on:
pull_request_target:
types: [opened, synchronize, reopened]Security note: When changing to pull_request_target, be aware that this runs in the context of the base repository with write permissions, so the checkout at line 23 is correct (checking out the PR head explicitly), but ensure the verify-pr.sh script doesn't execute any untrusted code from the PR.
| on: | |
| pull_request: | |
| types: [opened, synchronize, reopened] | |
| on: | |
| pull_request_target: | |
| types: [opened, synchronize, reopened] | |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
| exit 1 | ||
| fi | ||
|
|
||
| GIST_CONTENT=$(curl -s "https://api.github.com/gists/$GIST_ID" | jq -r '.files | to_entries[0].value.content // empty') |
There was a problem hiding this comment.
GitHub API rate limiting vulnerability: The script makes unauthenticated API calls to fetch gist content. For public repositories with many PRs, this will quickly hit GitHub's rate limit (60 requests/hour for unauthenticated requests), causing verification failures.
GIST_CONTENT=$(curl -s -H "Authorization: token $GITHUB_TOKEN" "https://api.github.com/gists/$GIST_ID" | jq -r '.files | to_entries[0].value.content // empty')The GITHUB_TOKEN should be passed from the GitHub Actions workflow (it's automatically available as ${{ secrets.GITHUB_TOKEN }}) as an environment variable to the script.
| GIST_CONTENT=$(curl -s "https://api.github.com/gists/$GIST_ID" | jq -r '.files | to_entries[0].value.content // empty') | |
| GIST_CONTENT=$(curl -s -H "Authorization: token $GITHUB_TOKEN" "https://api.github.com/gists/$GIST_ID" | jq -r '.files | to_entries[0].value.content // empty') |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
Metrics Comparison SummaryTotal changes across all snapshots: 37 Detailed changes per snapshotsummary_metrics_snapshot_cassandra📊 Metrics Diff SummaryTotal Changes: 0
summary_metrics_snapshot_cassandra📊 Metrics Diff SummaryTotal Changes: 0
summary_metrics_snapshot_cassandra📊 Metrics Diff SummaryTotal Changes: 0
summary_metrics_snapshot_elasticsearch📊 Metrics Diff SummaryTotal Changes: 37
🆕 Added Metrics
View diff sample+jaeger_storage_latency_seconds{le="+Inf",name="some_storage",operation="find_traces",otel_scope_name="jaeger-v2",otel_scope_schema_url="",otel_scope_version="",result="err",role="tracestore"}
+jaeger_storage_latency_seconds{le="0",name="some_storage",operation="find_traces",otel_scope_name="jaeger-v2",otel_scope_schema_url="",otel_scope_version="",result="err",role="tracestore"}
+jaeger_storage_latency_seconds{le="10",name="some_storage",operation="find_traces",otel_scope_name="jaeger-v2",otel_scope_schema_url="",otel_scope_version="",result="err",role="tracestore"}
+jaeger_storage_latency_seconds{le="100",name="some_storage",operation="find_traces",otel_scope_name="jaeger-v2",otel_scope_schema_url="",otel_scope_version="",result="err",role="tracestore"}
+jaeger_storage_latency_seconds{le="1000",name="some_storage",operation="find_traces",otel_scope_name="jaeger-v2",otel_scope_schema_url="",otel_scope_version="",result="err",role="tracestore"}
+jaeger_storage_latency_seconds{le="10000",name="some_storage",operation="find_traces",otel_scope_name="jaeger-v2",otel_scope_schema_url="",otel_scope_version="",result="err",role="tracestore"}
+jaeger_storage_latency_seconds{le="25",name="some_storage",operation="find_traces",otel_scope_name="jaeger-v2",otel_scope_schema_url="",otel_scope_version="",result="err",role="tracestore"}
...View diff sample+jaeger_storage_requests{name="some_storage",operation="find_traces",otel_scope_name="jaeger-v2",otel_scope_schema_url="",otel_scope_version="",result="err",role="tracestore"}View diff sample+rpc_server_call_duration_seconds{le="+Inf",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc",otel_scope_schema_url="https://opentelemetry.io/schemas/1.39.0",otel_scope_version="0.65.0",rpc_method="jaeger.api_v3.QueryService/FindTraces",rpc_response_status_code="Unknown",rpc_system_name="grpc"}
+rpc_server_call_duration_seconds{le="0",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc",otel_scope_schema_url="https://opentelemetry.io/schemas/1.39.0",otel_scope_version="0.65.0",rpc_method="jaeger.api_v3.QueryService/FindTraces",rpc_response_status_code="Unknown",rpc_system_name="grpc"}
+rpc_server_call_duration_seconds{le="10",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc",otel_scope_schema_url="https://opentelemetry.io/schemas/1.39.0",otel_scope_version="0.65.0",rpc_method="jaeger.api_v3.QueryService/FindTraces",rpc_response_status_code="Unknown",rpc_system_name="grpc"}
+rpc_server_call_duration_seconds{le="100",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc",otel_scope_schema_url="https://opentelemetry.io/schemas/1.39.0",otel_scope_version="0.65.0",rpc_method="jaeger.api_v3.QueryService/FindTraces",rpc_response_status_code="Unknown",rpc_system_name="grpc"}
+rpc_server_call_duration_seconds{le="1000",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc",otel_scope_schema_url="https://opentelemetry.io/schemas/1.39.0",otel_scope_version="0.65.0",rpc_method="jaeger.api_v3.QueryService/FindTraces",rpc_response_status_code="Unknown",rpc_system_name="grpc"}
+rpc_server_call_duration_seconds{le="10000",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc",otel_scope_schema_url="https://opentelemetry.io/schemas/1.39.0",otel_scope_version="0.65.0",rpc_method="jaeger.api_v3.QueryService/FindTraces",rpc_response_status_code="Unknown",rpc_system_name="grpc"}
+rpc_server_call_duration_seconds{le="25",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc",otel_scope_schema_url="https://opentelemetry.io/schemas/1.39.0",otel_scope_version="0.65.0",rpc_method="jaeger.api_v3.QueryService/FindTraces",rpc_response_status_code="Unknown",rpc_system_name="grpc"}
... |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #!/bin/bash | ||
| # Verify PR trailers for AI policy compliance. | ||
| # Called from GitHub Actions workflow. | ||
|
|
There was a problem hiding this comment.
Repository shell scripts typically include the project copyright + SPDX header near the top. This new script is missing it; please add the standard header to match the rest of scripts/.
| # Check for Tested-By trailer | ||
| if ! echo "$COMMIT_MSG" | grep -q "Tested-By:"; then | ||
| echo "❌ Missing Tested-By trailer." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Trailer detection is currently a substring grep, which can match Tested-By: / Test-Gist: anywhere in the message (not necessarily in the trailer block). Since this is meant as an enforcement mechanism, parse trailers explicitly (e.g., via git interpret-trailers --parse) and validate that the keys exist as actual trailers.
| #!/bin/bash | ||
| # Verify PR with proof for AI policy compliance. | ||
| # Uploads test logs to a Gist and adds trailers to the commit. | ||
|
|
There was a problem hiding this comment.
Repository shell scripts typically include the project copyright + SPDX header near the top. This new script is missing it; please add the standard header to match the rest of scripts/.
| TREE_SHA=$(git rev-parse HEAD'^{tree}') | ||
| echo "Tree SHA: $TREE_SHA" > .test.log.tmp | ||
| echo "---" >> .test.log.tmp | ||
| cat .test.log >> .test.log.tmp | ||
| mv .test.log.tmp .test.log | ||
|
|
There was a problem hiding this comment.
This script writes a .test.log.tmp file but only removes .test.log. If the script exits early (e.g., gh failure), the temp file can be left behind as an untracked artifact. Use a trap to clean up the temp file (and ideally avoid rewriting .test.log in-place).
| fi | ||
|
|
||
| NAME=$(git config user.name) | ||
| EMAIL=$(git config user.email) |
There was a problem hiding this comment.
NAME/EMAIL are read from git config but not validated. If either is unset/empty, the script will still amend the commit with an invalid Tested-By: value. Fail fast with a clear message if user.name or user.email is missing.
| EMAIL=$(git config user.email) | |
| EMAIL=$(git config user.email) | |
| if [ -z "$NAME" ] || [ -z "$EMAIL" ]; then | |
| echo "❌ Git user.name and/or user.email is not set." | |
| echo "Please configure them before running this script, for example:" | |
| echo " git config --global user.name \"Your Name\"" | |
| echo " git config --global user.email \"you@example.com\"" | |
| exit 1 | |
| fi |
| ASSOCIATION="${1:-}" | ||
| COMMIT_MSG=$(git log -1 --pretty=%B) | ||
|
|
There was a problem hiding this comment.
The verifier reads trailers from the current HEAD commit. To support a safer workflow that does not check out PR code, consider accepting the PR head SHA as an input and using git log -1 ... <sha> / git rev-parse <sha>^{tree}. This also avoids accidental verification of the wrong commit if the checkout logic changes.
| GIST_CONTENT=$(curl -s "https://api.github.com/gists/$GIST_ID" | jq -r '.files | to_entries[0].value.content // empty') | ||
| if [ -z "$GIST_CONTENT" ]; then | ||
| echo "❌ Could not fetch Gist" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
curl is called without --fail and without authentication, so API errors/rate limiting can be silently treated as "could not fetch" and may intermittently fail under load. Consider using curl -fsSL and passing an auth header (wire GITHUB_TOKEN from the workflow) to get higher rate limits and clearer failures.
|
|
||
|
|
There was a problem hiding this comment.
There are multiple consecutive blank lines added here. Please remove extras to keep spacing consistent with the rest of the Makefile.
| on: | ||
| pull_request: | ||
| types: [opened, synchronize, reopened] | ||
|
|
There was a problem hiding this comment.
The workflow is configured for pull_request, but the PR description says enforcement must run on pull_request_target to work reliably for fork PRs/new contributors. If the intent is technical enforcement for all PRs, switch the trigger to pull_request_target and adjust the checkout/verification accordingly (see separate comment about checking out PR code).
| - name: Checkout PR head | ||
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | ||
| with: | ||
| ref: ${{ github.event.pull_request.head.sha }} | ||
| fetch-depth: 1 | ||
|
|
||
| - name: Verify PR |
There was a problem hiding this comment.
The job checks out the PR head SHA and then runs ./scripts/verify-pr.sh from that checkout. For fork PRs, this means the PR author can modify scripts/verify-pr.sh (or any invoked tooling) to bypass verification, defeating the enforcement. Run the verifier from trusted code (base branch) and only fetch/inspect the untrusted commit by SHA (e.g., checkout base, git fetch the head SHA, and have the script read commit message/tree from that SHA without executing PR-provided scripts).
| - name: Checkout PR head | |
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | |
| with: | |
| ref: ${{ github.event.pull_request.head.sha }} | |
| fetch-depth: 1 | |
| - name: Verify PR | |
| - name: Checkout base commit | |
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | |
| with: | |
| ref: ${{ github.event.pull_request.base.sha }} | |
| fetch-depth: 1 | |
| - name: Verify PR | |
| env: | |
| PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} | |
| PR_AUTHOR_ASSOCIATION: ${{ github.event.pull_request.author_association }} |
Add SPDX license headers to verify-pr.sh and verify-with-proof.sh to pass the license check. Relates to jaegertracing#7878 Signed-off-by: Sapthagiri777 <sriramsapthagiri@gmail.com>
|
@yurishkuro Thanks for your patience with this PR! The license header issue is now fixed. Here's the current status:
Copilot has generated several more suggestions. I'd appreciate your guidance:
Happy to make whatever changes you think are necessary! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fi | ||
|
|
||
| # Check for Test-Gist trailer | ||
| GIST_URL=$(echo "$COMMIT_MSG" | grep "Test-Gist:" | awk '{print $2}' | head -1) |
There was a problem hiding this comment.
The grep pattern "Test-Gist:" and subsequent awk parsing could be fragile if there are multiple lines containing "Test-Gist:" or unusual spacing. Consider using git interpret-trailers --parse to properly extract the trailer value, or make the pattern more specific with "^Test-Gist: " to match only at line start.
| GIST_URL=$(echo "$COMMIT_MSG" | grep "Test-Gist:" | awk '{print $2}' | head -1) | |
| GIST_URL=$(printf '%s\n' "$COMMIT_MSG" | git interpret-trailers --parse | awk -F': ' '$1 == "Test-Gist" { print $2; exit }') |
| GIST_CONTENT=$(curl -s "https://api.github.com/gists/$GIST_ID" | jq -r '.files | to_entries[0].value.content // empty') | ||
| if [ -z "$GIST_CONTENT" ]; then | ||
| echo "❌ Could not fetch Gist" |
There was a problem hiding this comment.
If the curl command fails (network error, invalid URL, rate limiting, etc.), the error message on line 50 says "Could not fetch Gist" which is generic. Consider checking the HTTP response code and providing more specific error messages for different failure modes (e.g., 404 for non-existent gist, 403 for rate limiting, network errors).
| GIST_CONTENT=$(curl -s "https://api.github.com/gists/$GIST_ID" | jq -r '.files | to_entries[0].value.content // empty') | |
| if [ -z "$GIST_CONTENT" ]; then | |
| echo "❌ Could not fetch Gist" | |
| TMP_GIST_RESPONSE=$(mktemp) | |
| HTTP_RESPONSE=$(curl -s -w "%{http_code}" -o "$TMP_GIST_RESPONSE" "https://api.github.com/gists/$GIST_ID") || { | |
| echo "❌ Network error while fetching Gist" | |
| rm -f "$TMP_GIST_RESPONSE" | |
| exit 1 | |
| } | |
| HTTP_STATUS=${HTTP_RESPONSE: -3} | |
| if [ "$HTTP_STATUS" != "200" ]; then | |
| case "$HTTP_STATUS" in | |
| 404) | |
| echo "❌ Gist not found (404). Check the Test-Gist URL." | |
| ;; | |
| 403) | |
| echo "❌ Access to Gist forbidden or rate limited (403)." | |
| ;; | |
| *) | |
| echo "❌ Failed to fetch Gist. HTTP status: $HTTP_STATUS" | |
| ;; | |
| esac | |
| rm -f "$TMP_GIST_RESPONSE" | |
| exit 1 | |
| fi | |
| GIST_CONTENT=$(jq -r '.files | to_entries[0].value.content // empty' "$TMP_GIST_RESPONSE") | |
| rm -f "$TMP_GIST_RESPONSE" | |
| if [ -z "$GIST_CONTENT" ]; then | |
| echo "❌ Gist content is empty or in unexpected format." |
|
|
||
| set -euo pipefail | ||
|
|
||
| ASSOCIATION="${1:-}" |
There was a problem hiding this comment.
The script uses jq on line 48 to parse JSON from the GitHub API, but there's no check for whether jq is installed. Add a check similar to the gh CLI check in verify-with-proof.sh to ensure jq is available before attempting to use it.
| echo "Found Test-Gist: $GIST_URL" | ||
|
|
||
| # Extract and validate Gist | ||
| GIST_ID=$(echo "$GIST_URL" | grep -oE '[0-9a-f]{20,32}' | tail -1) |
There was a problem hiding this comment.
The regex pattern [0-9a-f]{20,32} for extracting Gist IDs is imprecise. GitHub Gist IDs are exactly 32 hexadecimal characters (128 bits). The pattern should be [0-9a-f]{32} to match only valid Gist IDs. The current pattern could accidentally match other hexadecimal strings in the URL.
| GIST_ID=$(echo "$GIST_URL" | grep -oE '[0-9a-f]{20,32}' | tail -1) | |
| GIST_ID=$(echo "$GIST_URL" | grep -oE '[0-9a-f]{32}' | tail -1) |
| ### Verification Workflow | ||
|
|
||
| Before pushing your PR, run: | ||
|
|
||
| ```bash | ||
| make verify-with-proof | ||
| ``` | ||
|
|
||
| Then push with `git push --force-with-lease`. |
There was a problem hiding this comment.
The verification workflow documentation is incomplete. It doesn't explain what the trailers are for, when they're required, or that new contributors need to authenticate with GitHub CLI (gh auth login) before running the command. Contributors may not understand why they're doing this or what happens if verification fails. Add explanations about the purpose, requirements (especially differentiating between trusted and new contributors), and the authentication prerequisite.
| GIST_URL=$(gh gist create .test.log -d "Test logs for Jaeger tree $TREE_SHA") | ||
| if [ -z "$GIST_URL" ]; then |
There was a problem hiding this comment.
The error check on line 37-40 only verifies if GIST_URL is empty, but doesn't check the exit code of the gh gist create command. If the command fails but still outputs something to stderr (which might be captured), the script could proceed incorrectly. Check the exit code explicitly using $? or use command substitution with proper error handling.
| GIST_URL=$(gh gist create .test.log -d "Test logs for Jaeger tree $TREE_SHA") | |
| if [ -z "$GIST_URL" ]; then | |
| if ! GIST_URL=$(gh gist create .test.log -d "Test logs for Jaeger tree $TREE_SHA"); then |
| fi | ||
|
|
||
| NAME=$(git config user.name) | ||
| EMAIL=$(git config user.email) |
There was a problem hiding this comment.
The script retrieves git user.name and user.email but doesn't verify they're set. If either is empty, the commit trailer will be malformed. Add validation to ensure both values are non-empty before using them, similar to how the script checks for other prerequisites.
| EMAIL=$(git config user.email) | |
| EMAIL=$(git config user.email) | |
| if [ -z "$NAME" ] || [ -z "$EMAIL" ]; then | |
| echo "❌ Git user.name and user.email must be set before running this script." | |
| echo " Configure them with, for example:" | |
| echo " git config --global user.name \"Your Name\"" | |
| echo " git config --global user.email \"you@example.com\"" | |
| exit 1 | |
| fi |
| make verify-with-proof | ||
| ``` | ||
|
|
||
| Then push with `git push --force-with-lease`. |
There was a problem hiding this comment.
Instructing users to run git push --force-with-lease could be risky if they don't understand what it does. If a contributor runs verify-with-proof multiple times or on the wrong commit, they might accidentally force-push over other people's changes. Consider adding a warning or explanation about when force-pushing is appropriate, or recommend creating the commit with the trailers from the beginning instead of amending.
|
|
||
| set -euo pipefail | ||
|
|
||
| ASSOCIATION="${1:-}" |
There was a problem hiding this comment.
The script doesn't validate that the ASSOCIATION parameter was actually provided. If called without arguments, ASSOCIATION will be empty and the is_trusted function will return false, which may work correctly by default, but it's better to explicitly check and fail with a clear error message if the required parameter is missing.
| ASSOCIATION="${1:-}" | |
| ASSOCIATION="${1:-}" | |
| if [ -z "$ASSOCIATION" ]; then | |
| echo "❌ Missing required PR association argument (e.g., OWNER, MEMBER, COLLABORATOR)." | |
| exit 1 | |
| fi |
| ### Verification Workflow | ||
|
|
||
| Before pushing your PR, run: | ||
|
|
||
| ```bash | ||
| make verify-with-proof | ||
| ``` | ||
|
|
||
| Then push with `git push --force-with-lease`. |
There was a problem hiding this comment.
The documentation doesn't mention that only new contributors need to provide the Test-Gist, while trusted contributors (OWNER, MEMBER, COLLABORATOR) only need the Tested-By trailer. This creates confusion because the instructions tell everyone to run make verify-with-proof, which will create a Gist even for trusted contributors who don't need it. Either update the documentation to explain this distinction, or consider whether all contributors should follow the same process for consistency.
|
@yurishkuro Thanks for your patience with this PR! Current status:
Copilot keeps generating suggestions (now 12 comments). The main ones are:
I'd appreciate your guidance:
Happy to make whatever changes you think are necessary! |
3 participants
Which problem is this PR solving?
This is the follow-up PR to #7932 (AI Usage Policy). As discussed in #7878, this PR adds the technical enforcement mechanisms.
Description of the changes
This PR implements the verification workflow that was discussed with @yurishkuro:
1. Makefile target:
make verify-with-proofContributors run this before pushing:
lintandtestghCLITested-By: Name <email>Test-Gist: https://gist.github.com/...2. GitHub Action using
pull_request_targetUses
author_associationto distinguish contributors:OWNER,MEMBER,COLLABORATOR): only requiresTested-By:FIRST_TIME_CONTRIBUTOR,FIRST_TIMER,NONE): requires bothTested-By:andTest-Gist:with valid GistRuns on
pull_request_targetso it works for PRs from forks3. Updated CONTRIBUTING_GUIDELINES.md
Added "Verification Workflow" section explaining:
make verify-with-proofWhy this approach?
As @yurishkuro pointed out:
Tested-By:is too easy to spoofpull_requestwon't run for new contributors, so we usepull_request_targetHow was this change tested?
Checklist
/cc @yurishkuro