Skip to content

Add automated PR checks and Copilot instructions for common contributor mistakes - #207

Merged
dalito merged 8 commits into
mainfrom
copilot/experiment-copilot-reviews
Nov 8, 2025
Merged

Add automated PR checks and Copilot instructions for common contributor mistakes#207
dalito merged 8 commits into
mainfrom
copilot/experiment-copilot-reviews

Conversation

Copilot AI commented Nov 8, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Implementation Complete: Automated PR Checks with Copilot Instructions ✅

This PR implements automated checks and GitHub Copilot instructions for common PR submission mistakes, addressing issue #206.

What Gets Checked

  • Main Branch Detection: Detects PRs from fork's main branch → Provides friendly guidance about feature branches
  • Organization Account Detection (CRITICAL): Warns about GitHub limitation that blocks CI - "Allow edits from maintainers" not available for org forks
  • Copilot Instructions: Guides GitHub Copilot to review for proper concept classification and other issues

Implementation Details

Files Created:

  • .github/copilot-instructions.md (150 lines) - Comprehensive guidance for Copilot reviews
  • .github/workflows/pr-checks.yml (189 lines) - Secure workflow with 2 checks
  • .github/workflows/SECURITY.md - Security documentation and mitigations
  • .github/workflows/README-pr-checks.md - Documentation and maintenance guide
  • .github/workflows/TESTING-pr-checks.md - Testing plan adapted for new approach
  • IMPLEMENTATION-SUMMARY.md - Complete summary

Files Modified:

  • CONTRIBUTING.md - Added best practices about feature branches

Changes from Initial Implementation

Per @dalito's feedback:

  • ✅ Removed Python-based top-concept validation (deferred to separate PR - Python in Actions hard to test)
  • ✅ Corrected org account explanation to focus on "Allow edits from maintainers" limitation (refs https://github.com/orgs/community/discussions/5634)
  • ✅ Added .github/copilot-instructions.md as expected approach for Copilot guidance
  • ✅ Fixed script injection vulnerabilities - Sanitized user-controlled data (usernames), changed console logging to avoid template literals
  • ✅ Simplified workflow from 2 jobs to 1 job with 2 checks (~1-2 min execution)

Security Mitigations

Per @dalito's security review:

  • Script injection prevention: Sanitize pr.user.login with regex to allow only alphanumeric + hyphens
  • Safe logging: Use comma-separated console.log instead of template literals with user data
  • Documentation: Added comprehensive SECURITY.md explaining all mitigations
  • Security header: Added comment in workflow explaining pull_request_target risks

Key Features

✅ Helpful, Not Blocking - Main branch check is informational; org account is critical (literally can't work)
✅ Friendly Tone - Comments are welcoming and educational
✅ No Spam - Comments posted only once
✅ Secure - Script injection prevention, minimal permissions, no code checkout
✅ Validated - CodeQL: 0 alerts
✅ Expected Approach - Uses Copilot instructions + minimal Actions

Technical Implementation

  • GitHub Actions workflow with JavaScript for checks
  • Security: Sanitizes all user-controlled data before use
  • Copilot instructions guide manual/AI review for classification
  • ~1-2 minute execution time
  • Single job with two check steps

Testing Status

  • ✅ Code is complete and ready for review
  • ✅ Security validated (CodeQL: 0 alerts, script injection mitigated)
  • ⏳ Awaiting real PRs to validate effectiveness
  • 📋 See TESTING-pr-checks.md for test scenarios
  • 📋 See SECURITY.md for security testing procedures

Next Steps

  1. Review and merge this PR
  2. Enable GitHub Copilot for repository (if not already)
  3. Monitor first few PRs with checks
  4. Consider top-concept validation in separate PR with proper testing

See IMPLEMENTATION-SUMMARY.md and SECURITY.md for complete details.

Original prompt

This section details on the original issue you should resolve

<issue_title>Experiment with Copilot reviews</issue_title>
<issue_description>While I don't like many AI reviews as they are lengthy and more "noise" than useful I hope that we can use it here to help with often made mistakes by newcomers or occasional GitHub users. For example we could look for and react on:

  • PRs submitted from the main branch of a fork --> explain why this is problematic and provide instructions to fix.
  • PRs submitted from an organization instead of a individual account. --> Fails due to GitHub issue
  • Missing classification under one of the top-concepts</issue_description>

<agent_instructions>Look at discussion in open and merged PRs to see what PR submitters did wrong.</agent_instructions>

Comments on the Issue (you are @copilot in this section)

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 4 commits November 8, 2025 10:59
@dalito
Add PR submission checks workflow
44c2922 
Co-authored-by: dalito <2648874+dalito@users.noreply.github.com>
@dalito
Document automated PR checks and update contributing guide
8e38d7e 
Co-authored-by: dalito <2648874+dalito@users.noreply.github.com>
@dalito
Add comprehensive testing plan for PR checks workflow
c8aefba 
Co-authored-by: dalito <2648874+dalito@users.noreply.github.com>
@dalito
Add implementation summary document
89acbb2 
Co-authored-by: dalito <2648874+dalito@users.noreply.github.com>
Copilot AI changed the title [WIP] Experiment with Copilot reviews for PR submissions Add automated PR submission checks for common contributor mistakes Nov 8, 2025
Copilot AI requested a review from dalito November 8, 2025 11:08
@dalito

dalito commented Nov 8, 2025

Copy link
Copy Markdown
Member

@copilot Interesting path to solve this. I agree that for clearly defined problems detecting the problem in an action is better than throwing an LLM at it.

Putting Python code into the action is problematic since it cannot be automatically tested easily. Defer the check that new concept may not be top-level to another PR.

Also the arguments why submission from an org does not work misses the main point: The option "Allow edits from maintainers" is not available for forks stored in an organization (see https://github.com/orgs/community/discussions/5634) but required for PRs in voc4cat else the action cannot commit the turtle files created from a submitted xlsx file and cannot remove the xlsx file.

I was expecting the creation of instructions for Copilot by adding a .github/copilot-instructions.md file to the repository as documented here https://docs.github.com/en/copilot/how-tos/configure-custom-instructions/add-repository-instructions

@dalito
Refactor PR checks based on maintainer feedback
6cc0338 
- Remove Python-based top-concept validation (defer to separate PR)
- Correct org account explanation: focus on "Allow edits from maintainers" GitHub limitation
- Add .github/copilot-instructions.md for GitHub Copilot review guidance
- Update documentation to reflect new approach
- Simplify workflow to 1 job with 2 checks (main branch + org account)

Co-authored-by: dalito <2648874+dalito@users.noreply.github.com>

Copilot AI commented Nov 8, 2025

Copy link
Copy Markdown
Contributor Author

@copilot Interesting path to solve this. I agree that for clearly defined problems detecting the problem in an action is better than throwing an LLM at it.

Putting Python code into the action is pro...

I've refactored the implementation based on your feedback in commit 6cc0338:

  1. ✅ Removed Python-based top-concept validation - Deferred to separate PR as you suggested since Python in Actions is hard to test
  2. ✅ Corrected org account explanation - Now focuses on the "Allow edits from maintainers" limitation (refs https://github.com/orgs/community/discussions/5634) and clearly states this blocks CI
  3. ✅ Added .github/copilot-instructions.md - Comprehensive guidance for Copilot reviews, including how to check for proper concept classification

The workflow is now simplified to 1 job with 2 checks (main branch + org account), taking ~1-2 minutes. Top-concept classification is handled via Copilot instructions for manual/AI review.

CodeQL: 0 alerts

Copilot AI changed the title Add automated PR submission checks for common contributor mistakes Add automated PR checks and Copilot instructions for common contributor mistakes Nov 8, 2025
@dalito

dalito commented Nov 8, 2025

Copy link
Copy Markdown
Member

@copilot On a quick look the added action is not safe against script-injection (https://github.com/actions/github-script#passing-inputs-to-the-script). Carefully review the action again for security flaws. Tools like zizmor may be of help.

@dalito
Fix script injection vulnerabilities in PR checks workflow
0f6d3ad 
- Sanitize pr.user.login to prevent injection (alphanumeric + hyphens only)
- Change console.log to use comma-separated values instead of template literals
- Add security documentation explaining mitigations
- Add security header comment in workflow file
- Update README with security references

Co-authored-by: dalito <2648874+dalito@users.noreply.github.com>

Copilot AI commented Nov 8, 2025

Copy link
Copy Markdown
Contributor Author

@copilot On a quick look the added action is not safe against script-injection (https://github.com/actions/github-script#passing-inputs-to-the-script). Carefully review the action again for security f...

Fixed script injection vulnerabilities in commit 0f6d3ad:

Mitigations implemented:

  • Sanitize pr.user.login with regex: replace(/[^a-zA-Z0-9-]/g, '') before use in template literals
  • Changed console.log to use comma-separated values instead of template literals with user data
  • Added security documentation in .github/workflows/SECURITY.md explaining all mitigations
  • Added security header comment in workflow file

What was vulnerable:

  • Direct interpolation of pr.user.login in lines 83 and 153 (now sanitized)
  • Template literals in console.log with user-controlled data (now comma-separated)

CodeQL: 0 alerts after fixes

@dalito
Clean-up copilot commits/suggestions
34a5185 
- remove excessive documentation
- tune messages
- avoid inconsistencies between action-review and agent-review
@dalito
dalito force-pushed the copilot/experiment-copilot-reviews branch from 7ad831f to 34a5185 Compare November 8, 2025 15:01
@dalito
dalito marked this pull request as ready for review November 8, 2025 15:03
@dalito
dalito requested a review from a team as a code owner November 8, 2025 15:03
@dalito

dalito commented Nov 8, 2025

Copy link
Copy Markdown
Member

I cleaned this up and removed the excessive documentation which was only relevant in the context of the joint PR development with the agent.

@dalito
dalito merged commit 9ff6d10 into main Nov 8, 2025
1 check passed
@github-project-automation github-project-automation Bot moved this from New to Completed in Voc4Cat cross-repo view Nov 8, 2025
@dalito
dalito deleted the copilot/experiment-copilot-reviews branch November 16, 2025 16:00
HendrikBorgelt pushed a commit to HendrikBorgelt/voc4cat that referenced this pull request Feb 13, 2026
@dalito @HendrikBorgelt
Add automated PR checks and Copilot instructions for common contribut…
a7ee176 
…or mistakes (nfdi4cat#207)

* Add PR submission checks workflow
* Add .github/copilot-instructions.md for GitHub Copilot review guidance

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: dalito <2648874+dalito@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@dalito dalito dalito approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@dalito dalito

Copilot code review Copilot

Labels

None yet

Projects

Voc4Cat cross-repo view
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Experiment with Copilot reviews

2 participants

@dalito