chore: add Copilot code review instructions#261
chore: add Copilot code review instructions#261Reese-max wants to merge 2 commits intoopenabdev:mainfrom
Conversation
Add `.github/copilot-instructions.md` to guide GitHub Copilot's automated PR reviews with project-specific context: - ACP protocol correctness (JSON-RPC routing, notification handling) - Concurrency safety (atomic fields, Mutex across await, pool locks) - Discord API constraints (2000 char limit, 3s autocomplete deadline) - Skip CI-covered checks (rustfmt, clippy, tests) - >80% confidence threshold to reduce noise Modeled after block/goose's production-grade instructions. 3440 chars — within the 4000 char limit.
There was a problem hiding this comment.
Pull request overview
Adds a repository-scoped .github/copilot-instructions.md to steer automated Copilot PR reviews toward OpenAB’s most important correctness/concurrency/protocol concerns and away from CI-covered noise.
Changes:
- Introduce Copilot review philosophy (high-confidence, concise, actionable).
- Document OpenAB-specific focus areas (ACP JSON-RPC correctness, async concurrency, Discord constraints, config/deployment).
- Add “Skip” guidance aligned with CI checks to reduce redundant comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot apply changes based on the comments in this thread |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ## CI Pipeline (Do Not Flag These) | ||
| - `cargo fmt --check` — formatting is enforced by CI | ||
| - `cargo clippy --all-targets -- -D warnings` — lint warnings are enforced by CI | ||
| - `cargo test` — test failures are caught by CI |
There was a problem hiding this comment.
The CI section is currently inaccurate: this repo’s CI runs cargo check, cargo clippy -- -D warnings, and cargo test, but does not run cargo fmt --check nor cargo clippy --all-targets (see .github/workflows/ci.yml). Consider updating this section (or adding a fmt check to CI) so Copilot doesn’t skip formatting/lint feedback based on a false assumption.
| - Race conditions in async code (especially in the reader loop and session pool) | ||
| - Resource leaks (child processes not killed, channels not closed) | ||
| - Off-by-one in timeout calculations | ||
| - Incorrect error propagation — `unwrap()` in non-test code is always a bug |
There was a problem hiding this comment.
The guidance "unwrap() in non-test code is always a bug" conflicts with the earlier note that some existing production code still uses unwrap/expect (e.g., src/config.rs:176, src/discord.rs:27/121), and would likely cause noisy reviews; consider narrowing it to flag newly introduced unwrap/expect in fallible paths rather than treating all uses as automatic bugs.
| - Incorrect error propagation — `unwrap()` in non-test code is always a bug | |
| - Incorrect error propagation — newly introduced `unwrap()`/`expect()` in fallible non-test production paths should usually be flagged unless failure is intentionally impossible and clearly justified |
| - Child process lifecycle — `kill_on_drop` must be set, zombie processes must not accumulate | ||
|
|
||
| ### ACP Protocol | ||
| - `session/request_permission` must always get a response (auto-allow or forwarded) |
There was a problem hiding this comment.
Given the repo already documents a real ACP interop pitfall where session/request_permission auto-replies without the required outcome wrapper (see docs/steering/triage.md and current implementation in src/acp/connection.rs), consider making this instruction more specific about validating the response shape (not just that a response exists) to prevent repeating that protocol bug.
| - `session/request_permission` must always get a response (auto-allow or forwarded) | |
| - `session/request_permission` must always get a protocol-valid response (auto-allow or forwarded); verify the response shape matches ACP expectations, including the required `outcome` wrapper, not just that some response exists |
Summary
Add
.github/copilot-instructions.mdto guide Copilot's automated PR reviews with OpenAB-specific context.Motivation
Copilot code review currently uses generic analysis. With project-specific instructions, it can focus on what matters for OpenAB:
Design Decisions
Modeled after block/goose's production-grade instructions (widely referenced as best practice).
Key choices:
Changes
.github/copilot-instructions.md