Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
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**: `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)
29 changes: 29 additions & 0 deletions src/acp/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,17 @@ pub struct AcpConnection {
pub acp_session_id: Option<String>,
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<std::sync::atomic::AtomicU64>,
pub context_size: Arc<std::sync::atomic::AtomicU64>,
Comment on lines +55 to +65
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 public fields context_used/context_size default to 0, which conflates “unknown/not provided by backend yet” with a legitimate 0 value. Since these are part of the connection’s public API surface, consider documenting the sentinel semantics (e.g., 0 == unknown) or representing “unknown” explicitly (Option/sentinel like u64::MAX) to avoid ambiguous downstream behavior.

Copilot uses AI. Check for mistakes.
Comment on lines +55 to +65
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.

context_used and context_size are updated via separate atomic stores, so readers can observe a mixed snapshot (new used with old size, or vice versa). Either document that these values are best-effort and may be temporarily inconsistent, or provide a helper that reads them consistently (e.g., read size, then used, then re-read size and retry if it changed).

Copilot uses AI. Check for mistakes.
_reader_handle: JoinHandle<()>,
}

Expand Down Expand Up @@ -86,11 +97,15 @@ impl AcpConnection {
Arc::new(Mutex::new(HashMap::new()));
let notify_tx: Arc<Mutex<Option<mpsc::UnboundedSender<JsonRpcMessage>>>> =
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();
Expand Down Expand Up @@ -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);
}
Comment on lines +150 to +157
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.

context_used and context_size are updated independently with relaxed atomics. Any reader that loads both to compute a ratio can observe a mixed snapshot (e.g., used from the new update with size from the previous one). If downstream consumers will read them as a pair, consider providing a snapshot getter using a version/sequence approach, or storing both values behind a small lock so reads are consistent.

Copilot uses AI. Check for mistakes.

// Response (has id) → resolve pending AND forward to subscriber
if let Some(id) = msg.id {
let mut map = pending.lock().await;
Expand Down Expand Up @@ -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,
})
}
Expand Down
6 changes: 6 additions & 0 deletions src/acp/protocol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<AcpEvent> {
Expand Down Expand Up @@ -105,6 +106,11 @@ pub fn classify_notification(msg: &JsonRpcMessage) -> Option<AcpEvent> {
}
}
"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,
}
}
71 changes: 69 additions & 2 deletions src/discord.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Comment on lines +600 to +608
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

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

The footer append check uses < 2000, so a chunk that would be exactly 2000 characters after appending the footer won’t get it; the follow-up >= 2000 check will then send the footer as a separate message even though it would have fit exactly. Consider using <= 2000 for the append condition and > 2000 (or equivalent) for the separate-message condition to avoid this off-by-one behavior.

Copilot uses AI. Check for mistakes.
} else {
chunk.to_string()
}
} else {
Comment on lines 597 to +612
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

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

Right now the context footer is only appended if it fits in the last chunk; when the final chunk is near the 2000-char limit, the footer is silently omitted, which contradicts “display it in Discord after each prompt response”. Consider emitting the footer as its own extra message/chunk when it doesn’t fit.

Copilot uses AI. Check for mistakes.
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;
}
}

Expand All @@ -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<String> {
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}%)"))
}
Comment thread
Reese-max marked this conversation as resolved.

/// 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,
Expand Down Expand Up @@ -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%"));
}
}
Loading