diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 0000000..caea1d7 --- /dev/null +++ b/.github/copilot-instructions.md @@ -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**: `anyhow::Result` everywhere, no `unwrap()` in production paths +- **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), `usage.rs` (pluggable quota runners), `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 + +### 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) +- `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: session/new=120s, all other methods (including initialize)=30s + +### Discord API +- Messages >2000 chars will be rejected — truncate or split +- Slash command registration is per-guild, max 100 per bot +- 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 + +## 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 +1. State the problem (1 sentence) +2. Why it matters (1 sentence, only if not obvious) +3. Suggested fix (code snippet or specific action) diff --git a/src/acp/connection.rs b/src/acp/connection.rs index 5377050..e36cb18 100644 --- a/src/acp/connection.rs +++ b/src/acp/connection.rs @@ -52,6 +52,17 @@ pub struct AcpConnection { pub acp_session_id: Option, pub last_active: Instant, pub session_reset: bool, + /// Context window usage from the latest `usage_update` ACP notification. + /// `context_used` may legitimately be 0 (e.g., new or empty context). + /// Treat `context_size == 0` as meaning no `usage_update` has been received yet. + /// + /// **Consistency note:** These two fields are updated via separate relaxed + /// atomic stores, so readers may observe a mixed pair (e.g., new `used` + /// with old `size`). This is acceptable for best-effort display purposes. + /// Code that requires a coordinated view of both values must use additional + /// synchronization instead of reading these atomics independently. + pub context_used: Arc, + pub context_size: Arc, _reader_handle: JoinHandle<()>, } @@ -86,11 +97,15 @@ impl AcpConnection { Arc::new(Mutex::new(HashMap::new())); let notify_tx: Arc>>> = Arc::new(Mutex::new(None)); + let context_used = Arc::new(std::sync::atomic::AtomicU64::new(0)); + let context_size = Arc::new(std::sync::atomic::AtomicU64::new(0)); let reader_handle = { let pending = pending.clone(); let notify_tx = notify_tx.clone(); let stdin_clone = stdin.clone(); + let ctx_used = context_used.clone(); + let ctx_size = context_size.clone(); tokio::spawn(async move { let mut reader = BufReader::new(stdout); let mut line = String::new(); @@ -129,6 +144,18 @@ impl AcpConnection { continue; } + // Capture usage_update for context window tracking. + // Reuses the shared classify_notification parser so the + // logic stays in one place (protocol.rs). + if let Some(crate::acp::protocol::AcpEvent::UsageUpdate { used, size }) = + crate::acp::protocol::classify_notification(&msg) + { + // Store size before used so readers that check + // `context_size == 0` as "no data yet" see size first. + ctx_size.store(size, Ordering::Relaxed); + ctx_used.store(used, Ordering::Relaxed); + } + // Response (has id) → resolve pending AND forward to subscriber if let Some(id) = msg.id { let mut map = pending.lock().await; @@ -186,6 +213,8 @@ impl AcpConnection { acp_session_id: None, last_active: Instant::now(), session_reset: false, + context_used, + context_size, _reader_handle: reader_handle, }) } diff --git a/src/acp/protocol.rs b/src/acp/protocol.rs index 82f00eb..76b0c5d 100644 --- a/src/acp/protocol.rs +++ b/src/acp/protocol.rs @@ -63,6 +63,7 @@ pub enum AcpEvent { ToolStart { id: String, title: String }, ToolDone { id: String, title: String, status: String }, Status, + UsageUpdate { used: u64, size: u64 }, } pub fn classify_notification(msg: &JsonRpcMessage) -> Option { @@ -105,6 +106,11 @@ pub fn classify_notification(msg: &JsonRpcMessage) -> Option { } } "plan" => Some(AcpEvent::Status), + "usage_update" => { + let used = update.get("used").and_then(|v| v.as_u64())?; + let size = update.get("size").and_then(|v| v.as_u64())?; + Some(AcpEvent::UsageUpdate { used, size }) + } _ => None, } } diff --git a/src/discord.rs b/src/discord.rs index e267064..bba7ac3 100644 --- a/src/discord.rs +++ b/src/discord.rs @@ -589,12 +589,40 @@ async fn stream_prompt( final_content }; + // Build context usage footer. + let ctx_size = conn.context_size.load(std::sync::atomic::Ordering::Relaxed); + let ctx_used = conn.context_used.load(std::sync::atomic::Ordering::Relaxed); + let ctx_footer = format_context_footer(ctx_used, ctx_size); + let chunks = format::split_message(&final_content, 2000); + let last_idx = chunks.len().saturating_sub(1); for (i, chunk) in chunks.iter().enumerate() { + // Append context footer to the last chunk if it fits (char count, not bytes). + // +1 accounts for the '\n' separator between chunk and footer. + let content = if i == last_idx { + if let Some(ref footer) = ctx_footer { + if chunk.chars().count() + 1 + footer.chars().count() <= 2000 { + format!("{chunk}\n{footer}") + } else { + chunk.to_string() + } + } else { + chunk.to_string() + } + } else { + chunk.to_string() + }; if i == 0 { - let _ = edit(&ctx, channel, current_msg_id, chunk).await; + let _ = edit(&ctx, channel, current_msg_id, &content).await; } else { - let _ = channel.say(&ctx.http, chunk).await; + let _ = channel.say(&ctx.http, &content).await; + } + } + // If footer didn't fit in the last chunk, send as a separate message. + if let Some(ref footer) = ctx_footer { + let last_chunk = chunks.last().map(|c| c.as_str()).unwrap_or(""); + if last_chunk.chars().count() + 1 + footer.chars().count() > 2000 { + let _ = channel.say(&ctx.http, footer).await; } } @@ -604,6 +632,18 @@ async fn stream_prompt( .await } +/// Format a context usage footer for Discord messages. +/// Returns `None` when `ctx_size == 0` (no `usage_update` received yet). +/// The percentage is clamped to 100 to handle `used > size` edge cases. +/// The returned string does NOT include a leading newline — callers add it as needed. +fn format_context_footer(ctx_used: u64, ctx_size: u64) -> Option { + if ctx_size == 0 { + return None; + } + let pct = ((ctx_used as u128 * 100 + ctx_size as u128 / 2) / ctx_size as u128).min(100) as u64; + Some(format!("-# 📊 Context: {ctx_used}/{ctx_size} tokens ({pct}%)")) +} + /// Flatten a tool-call title into a single line that's safe to render /// inside Discord inline-code spans. Discord renders single-backtick /// code on a single line only, so multi-line shell commands (heredocs, @@ -779,4 +819,31 @@ mod tests { let garbage = vec![0x00, 0x01, 0x02, 0x03]; assert!(resize_and_compress(&garbage).is_err()); } + + #[test] + fn context_footer_none_when_size_zero() { + assert!(format_context_footer(0, 0).is_none()); + assert!(format_context_footer(500, 0).is_none()); + } + + #[test] + fn context_footer_normal_percentage() { + let footer = format_context_footer(31434, 1_000_000).unwrap(); + assert!(footer.contains("Context: 31434/1000000")); + assert!(footer.contains("3%")); + assert!(!footer.starts_with('\n')); + } + + #[test] + fn context_footer_clamps_over_100() { + // used > size should clamp to 100% + let footer = format_context_footer(1_200_000, 1_000_000).unwrap(); + assert!(footer.contains("100%")); + } + + #[test] + fn context_footer_100_percent() { + let footer = format_context_footer(1_000_000, 1_000_000).unwrap(); + assert!(footer.contains("100%")); + } }