Skip to content

chore: add Copilot code review instructions #261

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.

Sign up for GitHub

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

Closed
Reese-max wants to merge 2 commits into from
Closed

chore: add Copilot code review instructions #261

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9230765
chore: add Copilot code review instructions
isxreese Apr 12, 2026
c2c9537
Apply suggestions from code review
Reese-max Apr 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions .github/copilot-instructions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
# GitHub Copilot Code Review Instructions

## Review Philosophy
- Only comment when you have HIGH CONFIDENCE (>80%) that an issue exists
- Be concise: one sentence per comment when possible
- Focus on actionable feedback, not observations
- Silence is preferred over noisy false positives

## Project Context
- **OpenAB**: A lightweight ACP (Agent Client Protocol) harness bridging Discord ↔ any ACP-compatible coding CLI over stdio JSON-RPC
- **Language**: Rust 2021 edition, single binary
- **Async runtime**: tokio (full features)
- **Discord**: serenity 0.12 (gateway + cache)
- **Error handling**: Prefer `anyhow::Result`; avoid `unwrap()` in production paths as a target, though some existing code may still use it
- **Serialization**: serde + serde_json for ACP JSON-RPC, toml for config
- **Key modules**: `acp/connection.rs` (ACP stdio bridge), `acp/pool.rs` (session pool), `discord.rs` (Discord event handler), `config.rs` (TOML config), `reactions.rs` (emoji reactions), `stt.rs` (speech-to-text)

## Priority Areas (Review These)

### Correctness
- Logic errors that could cause panics or incorrect behavior
- ACP JSON-RPC protocol violations (wrong method names, missing fields, incorrect response routing)
- 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
Copy link

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
- 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

Copilot uses AI. Check for mistakes.

### Concurrency & Safety
- Multiple atomic fields updated independently — document if readers may see mixed snapshots
- `Mutex` held across `.await` points (potential deadlock)
- Session pool lock scope — `RwLock` held during I/O can stall all sessions
- 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)
Copy link

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
- `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

Copilot uses AI. Check for mistakes.
- `session/update` notifications must not be consumed — forward to subscriber after capture
- `usage_update`, `available_commands_update`, `tool_call`, `agent_message_chunk` must be classified correctly
- Timeout values: initialize=30s, session/new=120s, others=30s

### Discord API
- Messages >2000 chars will be rejected — truncate or split
- Guild slash command registration limits are per application per guild; global command limits are separate
- Autocomplete responses must return within 3s (no heavy I/O)
- Ephemeral messages for errors, regular messages for results

### Config & Deployment
- `config.toml` fields must have sensible defaults — missing `[usage]` section should not crash
- Environment variable expansion via `${VAR}` must handle missing vars gracefully
- Agent `env` map is passed to child processes — sensitive values should not be logged

## 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
Comment on lines +51 to +54
Copy link

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

## Skip These (Low Value)
- Style/formatting — CI handles via rustfmt
- Clippy warnings — CI handles
- Minor naming suggestions unless truly confusing
- Suggestions to add comments for self-documenting code
- Logging level suggestions unless security-relevant
- Import ordering

## Response Format
- Default to a single concise sentence that states the problem and, when obvious, implies the fix
- Add one brief sentence explaining why it matters only when the impact is not obvious
- Include a code snippet or specific action only when it materially helps the author fix the issue
Loading