-
Notifications
You must be signed in to change notification settings - Fork 157
✨ Enhance Copilot instructions with comprehensive best practices #2221
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
Co-authored-by: benhillis <17727402+benhillis@users.noreply.github.com>
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.
Pull Request Overview
This PR enhances the GitHub Copilot instructions file to provide comprehensive context and guidance for the OpenVMM repository, expanding it from 29 to 104 lines following GitHub's best practices for Copilot coding agents.
Key Changes:
- Added structured sections for project overview, technology stack, and project structure to provide better repository context
- Expanded build, test, and formatting sections with concrete commands and examples
- Reorganized existing domain-specific security guidelines into a clearer structure
.github/copilot-instructions.md
Outdated
### Cross-compilation | ||
The project supports cross-compilation for `x86_64` and `aarch64` architectures on both Windows and Linux. See `cargo xflowey restore-packages --help` for cross-compilation package options. |
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.
- You can't build certain components from Windows (even if you're targeting Linux), you must use Linux. WSL counts as linux.
restore-packages --help
doesn't help with cross-compile. Look at the Guide (and see references for. ./build_support/setup_windows_cross.sh
as an example)
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.
Updated cross-compilation section to clarify that some components like OpenHCL can only be built from Linux (WSL2 counts as Linux), and corrected the reference to point to the Guide and the setup_windows_cross.sh script. (commit c5b3738)
### Required Before Each Commit | ||
Always run formatting before committing: | ||
```bash |
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.
You also want to run cargo doc
before committing, to catch any errors in the rustdocs.
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.
Added cargo doc
to the required checks before committing to catch rustdoc errors. (commit c5b3738)
.github/copilot-instructions.md
Outdated
```bash | ||
# Recommended - install with: cargo install cargo-nextest --locked | ||
cargo nextest run | ||
|
||
# Or use standard cargo test | ||
cargo test |
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.
Note that the default won't actually run anything. You should run the tests in the packages that you are modifying. vmm-tests
is a special case and requires additional setup.
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.
You likely also want to use the /.config/nextest.toml file to configure how you run tests.
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.
Updated testing section to clarify that default won't run anything and users should specify packages with -p <package-name>
. Also added a note about using .config/nextest.toml
for test configuration. (commit c5b3738)
.github/copilot-instructions.md
Outdated
### Domain-specific Guidelines | ||
Both OpenVMM and OpenHCL process data from untrusted sources. OpenHCL runs in a constrained environment. | ||
|
||
When possible: | ||
1. Avoid `unsafe` code | ||
2. Avoid taking new external dependencies, especially those that significantly increase binary size | ||
3. Ensure code doesn't panic across trust boundaries (critical for security) |
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 has tripped me up with copilot-generated and copilot-reviewed code. We should better clarify the trust boundary:
- OpenVMM does not trust the VTL0 guest.
- OpenHCL does not trust the root.
- OpenHCL does not trust the VTL0 guest, but this is less critical than OpenVMM not trusting the VTL0 guest. (the attack surface is subtle, so needs human review).
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.
Added explicit trust boundary clarifications:
- OpenVMM does not trust the VTL0 guest
- OpenHCL does not trust the root
- OpenHCL does not trust the VTL0 guest (less critical, subtle attack surface needs human review)
This should help Copilot better understand and respect these critical security boundaries. (commit c5b3738)
@copilot, I left some comments for you. |
…ries, and add cargo doc Co-authored-by: mattkur <13772048+mattkur@users.noreply.github.com>
Original prompt
Fixes #2220
💡 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.