wip - #1
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a Discord bot using the poise and serenity frameworks, introducing workspace-level dependency management and initial slash commands for pinging and issue tracking. Feedback identifies that the binary entry point is incorrectly placed in lib.rs instead of main.rs. Furthermore, the MESSAGE_CONTENT gateway intent should be removed as it is a privileged intent unnecessary for slash commands, and the issue command group must be registered in the framework options to be accessible in Discord.
| #[tokio::main] | ||
| async fn main() { |
There was a problem hiding this comment.
The main function and #[tokio::main] attribute are located in src/lib.rs. In Rust, src/lib.rs is typically reserved for library code, while the entry point for a binary should be src/main.rs. As it stands, cargo run will not execute this main function unless the crate is specifically configured as a binary in Cargo.toml. Consider renaming this file to src/main.rs or moving the entry point logic.
| let intents | ||
| = GatewayIntents::GUILD_MESSAGES | GatewayIntents::DIRECT_MESSAGES | GatewayIntents::MESSAGE_CONTENT; |
There was a problem hiding this comment.
The MESSAGE_CONTENT intent is a privileged intent that requires manual activation in the Discord Developer Portal and is generally discouraged unless strictly necessary. Since this bot appears to rely on slash commands (which do not require this intent), it should be removed to follow the principle of least privilege and avoid potential verification issues.
| let intents | |
| = GatewayIntents::GUILD_MESSAGES | GatewayIntents::DIRECT_MESSAGES | GatewayIntents::MESSAGE_CONTENT; | |
| let intents | |
| = GatewayIntents::GUILD_MESSAGES | GatewayIntents::DIRECT_MESSAGES; |
| commands: vec![ | ||
| commands::ping() | ||
| ], |
There was a problem hiding this comment.
The issue command group is defined in the commands module but is not registered in the FrameworkOptions. Only the ping command is currently registered, meaning the issue-related slash commands will not be available in Discord.
| commands: vec![ | |
| commands::ping() | |
| ], | |
| commands: vec![ | |
| commands::ping(), | |
| commands::issue(), | |
| ], |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new Discord issue-triage bot and its corresponding Axum backend service, featuring PostgreSQL integration, migrations, and an AI-assisted issue drafting flow. The feedback highlights several critical issues that need to be addressed: the use of unstable let-chains and non-existent SQLx error helper methods that will cause compilation failures on stable Rust, potential database transaction rollbacks due to duplicate assignees, an unnecessary database roundtrip in the comment creation handler, and a lack of resilience to bot restarts in the draft confirmation flow.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if let Err(sqlx::Error::Database(e)) = &insert | ||
| && e.is_unique_violation() | ||
| { | ||
| return Ok((StatusCode::CONFLICT, "already a zako").into_response()); | ||
| } | ||
| insert?; |
There was a problem hiding this comment.
The use of let-chains (&& with let) is unstable in Rust and requires the #![feature(let_chains)] compiler flag. Additionally, sqlx::error::DatabaseError does not have an is_unique_violation() method. To check for a unique constraint violation in PostgreSQL, you should check if the error code is "23505".
| if let Err(sqlx::Error::Database(e)) = &insert | |
| && e.is_unique_violation() | |
| { | |
| return Ok((StatusCode::CONFLICT, "already a zako").into_response()); | |
| } | |
| insert?; | |
| if let Err(sqlx::Error::Database(e)) = &insert { | |
| if e.code().as_deref() == Some("23505") { | |
| return Ok((StatusCode::CONFLICT, "already a zako").into_response()); | |
| } | |
| } | |
| insert?; |
| if let Err(sqlx::Error::Database(e)) = &result | ||
| && e.is_foreign_key_violation() | ||
| { | ||
| return Err(AppError::NotFound(format!("issue {id} not found"))); | ||
| } | ||
| result?; |
There was a problem hiding this comment.
The use of let-chains is unstable in Rust. Additionally, sqlx::error::DatabaseError does not have an is_foreign_key_violation() method. To check for a foreign key constraint violation in PostgreSQL, you should check if the error code is "23503".
| if let Err(sqlx::Error::Database(e)) = &result | |
| && e.is_foreign_key_violation() | |
| { | |
| return Err(AppError::NotFound(format!("issue {id} not found"))); | |
| } | |
| result?; | |
| if let Err(sqlx::Error::Database(e)) = &result { | |
| if e.code().as_deref() == Some("23503") { | |
| return Err(AppError::NotFound(format!("issue {id} not found"))); | |
| } | |
| } | |
| result?; |
| if let Some(issue_channel) = data.config.issue_channel_id | ||
| && ctx.channel_id() != issue_channel | ||
| { | ||
| return Err("This command must be used in the issue channel.".to_string()); | ||
| } |
There was a problem hiding this comment.
The use of let-chains is unstable in Rust. Rewrite this condition using nested if statements to ensure compatibility with stable Rust.
| if let Some(issue_channel) = data.config.issue_channel_id | |
| && ctx.channel_id() != issue_channel | |
| { | |
| return Err("This command must be used in the issue channel.".to_string()); | |
| } | |
| if let Some(issue_channel) = data.config.issue_channel_id { | |
| if ctx.channel_id() != issue_channel { | |
| return Err("This command must be used in the issue channel.".to_string()); | |
| } | |
| } |
| let exists: bool = sqlx::query_scalar("SELECT EXISTS(SELECT 1 FROM issues WHERE id = $1)") | ||
| .bind(issue_id) | ||
| .fetch_one(&state.pool) | ||
| .await?; | ||
| if !exists { | ||
| return Err(AppError::NotFound(format!("issue {issue_id} not found"))); | ||
| } | ||
|
|
||
| let comment = sqlx::query_as::<_, Comment>( | ||
| "INSERT INTO comments (issue_id, author, content) VALUES ($1, $2, $3) \ | ||
| RETURNING id, issue_id, author, content, created_at, updated_at", | ||
| ) | ||
| .bind(issue_id) | ||
| .bind(&req.author) | ||
| .bind(&req.content) | ||
| .fetch_one(&state.pool) | ||
| .await?; |
There was a problem hiding this comment.
Instead of performing an extra SELECT EXISTS query to check if the issue exists, you can perform the INSERT directly and handle the foreign key constraint violation error (23503). This avoids an unnecessary database roundtrip and eliminates a potential race condition.
| let exists: bool = sqlx::query_scalar("SELECT EXISTS(SELECT 1 FROM issues WHERE id = $1)") | |
| .bind(issue_id) | |
| .fetch_one(&state.pool) | |
| .await?; | |
| if !exists { | |
| return Err(AppError::NotFound(format!("issue {issue_id} not found"))); | |
| } | |
| let comment = sqlx::query_as::<_, Comment>( | |
| "INSERT INTO comments (issue_id, author, content) VALUES ($1, $2, $3) \ | |
| RETURNING id, issue_id, author, content, created_at, updated_at", | |
| ) | |
| .bind(issue_id) | |
| .bind(&req.author) | |
| .bind(&req.content) | |
| .fetch_one(&state.pool) | |
| .await?; | |
| let comment = sqlx::query_as::<_, Comment>( | |
| "INSERT INTO comments (issue_id, author, content) VALUES ($1, $2, $3) \ | |
| RETURNING id, issue_id, author, content, created_at, updated_at", | |
| ) | |
| .bind(issue_id) | |
| .bind(&req.author) | |
| .bind(&req.content) | |
| .fetch_one(&state.pool) | |
| .await; | |
| match comment { | |
| Ok(c) => Ok((StatusCode::CREATED, Json(c)).into_response()), | |
| Err(sqlx::Error::Database(e)) if e.code().as_deref() == Some("23503") => { | |
| Err(AppError::NotFound(format!("issue {issue_id} not found"))) | |
| } | |
| Err(e) => Err(e.into()), | |
| } |
| for assignee in &req.assignees { | ||
| sqlx::query("INSERT INTO issue_assignees (issue_id, author_id) VALUES ($1, $2)") | ||
| .bind(issue.id) | ||
| .bind(assignee) | ||
| .execute(&mut *tx) | ||
| .await?; | ||
| } |
There was a problem hiding this comment.
If req.assignees contains duplicate user IDs, inserting them sequentially will trigger a unique constraint violation on the issue_assignees primary key, causing the entire transaction to roll back. Use ON CONFLICT DO NOTHING to gracefully handle duplicate assignees.
| for assignee in &req.assignees { | |
| sqlx::query("INSERT INTO issue_assignees (issue_id, author_id) VALUES ($1, $2)") | |
| .bind(issue.id) | |
| .bind(assignee) | |
| .execute(&mut *tx) | |
| .await?; | |
| } | |
| for assignee in &req.assignees { | |
| sqlx::query("INSERT INTO issue_assignees (issue_id, author_id) VALUES ($1, $2) ON CONFLICT DO NOTHING") | |
| .bind(issue.id) | |
| .bind(assignee) | |
| .execute(&mut *tx) | |
| .await?; | |
| } |
| for assignee in assignees { | ||
| sqlx::query("INSERT INTO issue_assignees (issue_id, author_id) VALUES ($1, $2)") | ||
| .bind(id) | ||
| .bind(assignee) | ||
| .execute(&mut *tx) | ||
| .await?; | ||
| } |
There was a problem hiding this comment.
If assignees contains duplicate user IDs, inserting them sequentially will trigger a unique constraint violation on the issue_assignees primary key, causing the entire transaction to roll back. Use ON CONFLICT DO NOTHING to gracefully handle duplicate assignees.
| for assignee in assignees { | |
| sqlx::query("INSERT INTO issue_assignees (issue_id, author_id) VALUES ($1, $2)") | |
| .bind(id) | |
| .bind(assignee) | |
| .execute(&mut *tx) | |
| .await?; | |
| } | |
| for assignee in assignees { | |
| sqlx::query("INSERT INTO issue_assignees (issue_id, author_id) VALUES ($1, $2) ON CONFLICT DO NOTHING") | |
| .bind(id) | |
| .bind(assignee) | |
| .execute(&mut *tx) | |
| .await?; | |
| } |
| let state = data.drafts.lock().await.remove(&channel); | ||
| let Some(state) = state else { | ||
| ack(ctx, mci, "This draft is no longer active.").await?; | ||
| return Ok(()); | ||
| }; | ||
|
|
||
| let issue = match data.api.confirm_draft(state.draft_id).await { | ||
| Ok(issue) => issue, | ||
| Err(e) => { | ||
| // Keep the draft so the user can retry. | ||
| data.drafts.lock().await.insert(channel, state); | ||
| ack(ctx, mci, &format!("Could not create the issue: {e}")).await?; | ||
| return Ok(()); | ||
| } | ||
| }; |
There was a problem hiding this comment.
If the bot is restarted, the in-memory data.drafts map is cleared. Consequently, any active draft confirmation buttons will fail with "This draft is no longer active.". To make this robust across restarts, parse the draft_id from the button's custom_id and use it as a fallback when the in-memory state is missing.
let state = data.drafts.lock().await.remove(&channel);
let (backend_draft_id, original_ref) = match &state {
Some(s) => (s.draft_id, s.original_ref.clone()),
None => {
let parsed_id = mci.data.custom_id.strip_prefix("issue_confirm:")
.and_then(|id| id.parse::<i64>().ok());
match parsed_id {
Some(id) => (id, None),
None => {
ack(ctx, mci, "This draft is no longer active.").await?;
return Ok(());
}
}
}
};
let issue = match data.api.confirm_draft(backend_draft_id).await {
Ok(issue) => issue,
Err(e) => {
if let Some(s) = state {
data.drafts.lock().await.insert(channel, s);
}
ack(ctx, mci, &format!("Could not create the issue: {e}")).await?;
return Ok(());
}
};
|
/attacca review |
|
✅ Review posted below. |
![attaccacode[bot]](https://avatars.githubusercontent.com/in/4253958?s=60&v=4){kind=small}
There was a problem hiding this comment.
Attacca Code Review
(No overall summary available.)
ℹ️ 5 additional file(s) were not reviewed (limit 30).
No issues found at or above the LOW severity threshold.
|
/attacca review |
|
⏳ Still working on this — taking longer than usual, please wait... |
1 similar comment
|
⏳ Still working on this — taking longer than usual, please wait... |
|
/attacca review |
|
✅ Review posted below. |
There was a problem hiding this comment.
Attacca Code Review
(No overall summary available.)
ℹ️ 5 additional file(s) were not reviewed (limit 30).
No issues found at or above the LOW severity threshold.
|
/attacca review |
|
✅ Review posted below. |
There was a problem hiding this comment.
Attacca Code Review
(No overall summary available.)
ℹ️ 5 additional file(s) were not reviewed (limit 30).
No issues found at or above the LOW severity threshold.
|
/attacca review |
Attacca Code ReviewReason: Attacca could not reach the AI service at its configured address. Re-run with |
|
/attacca review |
Attacca Code ReviewReason: Attacca could not reach the AI service at its configured address. Re-run with |
|
/attacca review |
Attacca Code ReviewReason: Attacca could not reach the AI service at its configured address. Re-run with |
|
/attacca review |
Attacca Code ReviewReason: Attacca's AI credentials are missing or lack the required scopes — a repo admin needs to check the deployment. Re-run with |
|
/attacca review |
|
⏳ Still working on this — taking longer than usual, please wait... |
|
✅ Review posted below. |
There was a problem hiding this comment.
Attacca Code Review
This PR replaces the SeaORM scaffolding with a working two-service system: an axum + sqlx backend (issues, zakonim, Discord message mirroring, AI summary drafts via Anthropic/OpenAI-compatible APIs) and a poise/serenity Discord bot (slash commands, context-menu issue creation, per-user draft channels, Redis author cache), plus Docker/compose deployment. The overall structure is sound and the SQL/transaction usage is mostly careful, but the backend exposes every mutation endpoint with no authentication while compose publishes it on the host, the message-mirror upsert can null out stored content, the draft-summary handler persists user messages before the LLM call (breaking retries and Anthropic's alternating-role requirement on failure), the context-menu command never defers before a long LLM round trip so Discord drops the interaction, and the confirm flow loses the draft and silently errors if posting the issue fails.
Posted 6 finding(s) inline below.
| let app = Router::new() | ||
| .route("/", get(handlers::root::root)) | ||
| .route( | ||
| "/zakonim", | ||
| get(handlers::zakonim::list).post(handlers::zakonim::register), | ||
| ) | ||
| .route( | ||
| "/issues", | ||
| get(handlers::issue::list).post(handlers::issue::create), | ||
| ) | ||
| .route( | ||
| "/issues/{id}", | ||
| get(handlers::issue::get).patch(handlers::issue::edit), | ||
| ) | ||
| .route( | ||
| "/issues/{id}/comments", | ||
| get(handlers::comment::list).post(handlers::comment::create), | ||
| ) | ||
| .route( | ||
| "/issues/{id}/discord-messages/{key}", | ||
| put(handlers::issue::set_discord_message), | ||
| ) | ||
| .route( | ||
| "/discord/messages/{message_id}", | ||
| put(handlers::discord::upsert).delete(handlers::discord::delete), | ||
| ) | ||
| .route("/summary/drafts", post(handlers::summary::create)) | ||
| .route( | ||
| "/summary/drafts/{id}/messages", | ||
| post(handlers::summary::message), | ||
| ) | ||
| .route( | ||
| "/summary/drafts/{id}/confirm", | ||
| post(handlers::summary::confirm), | ||
| ) | ||
| .with_state(state); |
There was a problem hiding this comment.
🟠 HIGH · Security
Every route — creating/editing/closing issues, mirroring Discord events, and the /summary/drafts/* endpoints that trigger paid LLM calls — is registered with no authentication, and compose.yml publishes the service to 0.0.0.0:8080. Any host that can reach the port can spoof issue authors/assignees, close or edit any issue, inject arbitrary message_events, and drive unbounded LLM API usage billed to the operator's key. Add a shared-secret check (e.g. an X-API-Key header validated by middleware, with the bot sending the same secret) before exposing this.
| let app = Router::new() | |
| .route("/", get(handlers::root::root)) | |
| .route( | |
| "/zakonim", | |
| get(handlers::zakonim::list).post(handlers::zakonim::register), | |
| ) | |
| .route( | |
| "/issues", | |
| get(handlers::issue::list).post(handlers::issue::create), | |
| ) | |
| .route( | |
| "/issues/{id}", | |
| get(handlers::issue::get).patch(handlers::issue::edit), | |
| ) | |
| .route( | |
| "/issues/{id}/comments", | |
| get(handlers::comment::list).post(handlers::comment::create), | |
| ) | |
| .route( | |
| "/issues/{id}/discord-messages/{key}", | |
| put(handlers::issue::set_discord_message), | |
| ) | |
| .route( | |
| "/discord/messages/{message_id}", | |
| put(handlers::discord::upsert).delete(handlers::discord::delete), | |
| ) | |
| .route("/summary/drafts", post(handlers::summary::create)) | |
| .route( | |
| "/summary/drafts/{id}/messages", | |
| post(handlers::summary::message), | |
| ) | |
| .route( | |
| "/summary/drafts/{id}/confirm", | |
| post(handlers::summary::confirm), | |
| ) | |
| .with_state(state); | |
| let app = Router::new() | |
| .route("/", get(handlers::root::root)) | |
| .route( | |
| "/zakonim", | |
| get(handlers::zakonim::list).post(handlers::zakonim::register), | |
| ) | |
| .route( | |
| "/issues", | |
| get(handlers::issue::list).post(handlers::issue::create), | |
| ) | |
| .route( | |
| "/issues/{id}", | |
| get(handlers::issue::get).patch(handlers::issue::edit), | |
| ) | |
| .route( | |
| "/issues/{id}/comments", | |
| get(handlers::comment::list).post(handlers::comment::create), | |
| ) | |
| .route( | |
| "/issues/{id}/discord-messages/{key}", | |
| put(handlers::issue::set_discord_message), | |
| ) | |
| .route( | |
| "/discord/messages/{message_id}", | |
| put(handlers::discord::upsert).delete(handlers::discord::delete), | |
| ) | |
| .route("/summary/drafts", post(handlers::summary::create)) | |
| .route( | |
| "/summary/drafts/{id}/messages", | |
| post(handlers::summary::message), | |
| ) | |
| .route( | |
| "/summary/drafts/{id}/confirm", | |
| post(handlers::summary::confirm), | |
| ) | |
| .with_state(state.clone()) | |
| .layer(axum::middleware::from_fn_with_state(state, require_api_key)); | |
| // async fn require_api_key( | |
| // State(state): State<AppState>, | |
| // req: axum::extract::Request, | |
| // next: axum::middleware::Next, | |
| // ) -> Result<axum::response::Response, axum::response::Response> { | |
| // let secret = std::env::var("API_KEY").unwrap_or_default(); | |
| // if !secret.is_empty() | |
| // && req.headers() | |
| // .get("x-api-key") | |
| // .and_then(|v| v.to_str().ok()) | |
| // != Some(secret.as_str()) | |
| // { | |
| // return Err(axum::response::Response::new( | |
| // axum::body::Body::from("unauthorized"), | |
| // )); | |
| // } | |
| // Ok(next.run(req).await) | |
| // } |
| content = EXCLUDED.content, \ | ||
| edited_at = EXCLUDED.edited_at \ |
There was a problem hiding this comment.
🟡 MEDIUM · Correctness
The upsert blindly overwrites content with EXCLUDED.content, but the bot's MessageUpdateEvent handler sends content: None whenever an edit event doesn't carry the new text (e.g. embed-only or metadata updates in serenity 0.12). Such an event nulls out the previously mirrored message content. The same applies to edited_at: propagate_create sends edited_at: None, so a re-create of an already-known message clears the stored edit timestamp. Use COALESCE to preserve existing values when the incoming payload omits them.
| content = EXCLUDED.content, \ | |
| edited_at = EXCLUDED.edited_at \ | |
| content = COALESCE(EXCLUDED.content, message_events.content), \ | |
| edited_at = COALESCE(EXCLUDED.edited_at, message_events.edited_at) \ |
| sqlx::query( | ||
| "INSERT INTO summary_draft_messages (draft_id, role, content) VALUES ($1, 'user', $2)", | ||
| ) | ||
| .bind(id) | ||
| .bind(&req.content) | ||
| .execute(&state.pool) | ||
| .await?; | ||
|
|
||
| let history = sqlx::query_as::<_, DraftMessage>( | ||
| "SELECT role, content FROM summary_draft_messages WHERE draft_id = $1 ORDER BY id ASC", | ||
| ) | ||
| .bind(id) | ||
| .fetch_all(&state.pool) | ||
| .await?; | ||
| let transcript: Vec<(String, String)> = | ||
| history.into_iter().map(|m| (m.role, m.content)).collect(); | ||
|
|
||
| let reply = llm::reply(&state.http, &state.llm, &transcript).await?; | ||
|
|
||
| let mut tx = state.pool.begin().await?; | ||
| sqlx::query( | ||
| "INSERT INTO summary_draft_messages (draft_id, role, content) VALUES ($1, 'assistant', $2)", | ||
| ) | ||
| .bind(id) | ||
| .bind(&reply) | ||
| .execute(&mut *tx) | ||
| .await?; | ||
| sqlx::query("UPDATE summary_drafts SET summary = $2 WHERE id = $1") | ||
| .bind(id) | ||
| .bind(&reply) | ||
| .execute(&mut *tx) | ||
| .await?; | ||
| tx.commit().await?; |
There was a problem hiding this comment.
🟡 MEDIUM · Correctness
The user's message is committed to summary_draft_messages before the LLM call. If llm::reply fails (network error, provider 5xx, or — per .env.example — an unset API key), the 502 leaves the user message persisted with no assistant reply; when the bot retries, the message is inserted a second time, and two consecutive 'user' rows violate the Anthropic Messages API's alternating-roles requirement (the provider rejects the request with 400). Because the history SELECT runs on a separate pool connection, it would not see a still-open transaction's insert, so the whole flow must run on one connection: begin the transaction before inserting the user message and commit only after the assistant reply and summary update succeed.
| sqlx::query( | |
| "INSERT INTO summary_draft_messages (draft_id, role, content) VALUES ($1, 'user', $2)", | |
| ) | |
| .bind(id) | |
| .bind(&req.content) | |
| .execute(&state.pool) | |
| .await?; | |
| let history = sqlx::query_as::<_, DraftMessage>( | |
| "SELECT role, content FROM summary_draft_messages WHERE draft_id = $1 ORDER BY id ASC", | |
| ) | |
| .bind(id) | |
| .fetch_all(&state.pool) | |
| .await?; | |
| let transcript: Vec<(String, String)> = | |
| history.into_iter().map(|m| (m.role, m.content)).collect(); | |
| let reply = llm::reply(&state.http, &state.llm, &transcript).await?; | |
| let mut tx = state.pool.begin().await?; | |
| sqlx::query( | |
| "INSERT INTO summary_draft_messages (draft_id, role, content) VALUES ($1, 'assistant', $2)", | |
| ) | |
| .bind(id) | |
| .bind(&reply) | |
| .execute(&mut *tx) | |
| .await?; | |
| sqlx::query("UPDATE summary_drafts SET summary = $2 WHERE id = $1") | |
| .bind(id) | |
| .bind(&reply) | |
| .execute(&mut *tx) | |
| .await?; | |
| tx.commit().await?; | |
| let mut tx = state.pool.begin().await?; | |
| sqlx::query( | |
| "INSERT INTO summary_draft_messages (draft_id, role, content) VALUES ($1, 'user', $2)", | |
| ) | |
| .bind(id) | |
| .bind(&req.content) | |
| .execute(&mut *tx) | |
| .await?; | |
| let history = sqlx::query_as::<_, DraftMessage>( | |
| "SELECT role, content FROM summary_draft_messages WHERE draft_id = $1 ORDER BY id ASC", | |
| ) | |
| .bind(id) | |
| .fetch_all(&mut *tx) | |
| .await?; | |
| let transcript: Vec<(String, String)> = | |
| history.into_iter().map(|m| (m.role, m.content)).collect(); | |
| let reply = llm::reply(&state.http, &state.llm, &transcript).await?; | |
| sqlx::query( | |
| "INSERT INTO summary_draft_messages (draft_id, role, content) VALUES ($1, 'assistant', $2)", | |
| ) | |
| .bind(id) | |
| .bind(&reply) | |
| .execute(&mut *tx) | |
| .await?; | |
| sqlx::query("UPDATE summary_drafts SET summary = $2 WHERE id = $1") | |
| .bind(id) | |
| .bind(&reply) | |
| .execute(&mut *tx) | |
| .await?; | |
| tx.commit().await?; |
| let temp = start_draft( | ||
| ctx.serenity_context(), | ||
| data, | ||
| guild_id, | ||
| author, | ||
| None, | ||
| Some(message.content.clone()), | ||
| Some(original), | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
🟡 MEDIUM · Correctness
The "Create Issue" context-menu command runs start_draft — which makes a backend call that performs a full LLM round trip (potentially tens of seconds with claude-opus) — before sending any response to the interaction. Discord drops interactions that go unacknowledged for 3 seconds, so ctx.send fails, the user never receives the ephemeral link to the draft channel, and an untracked draft channel is left behind. Defer the interaction first (poise does not auto-defer).
| let temp = start_draft( | |
| ctx.serenity_context(), | |
| data, | |
| guild_id, | |
| author, | |
| None, | |
| Some(message.content.clone()), | |
| Some(original), | |
| ) | |
| .await?; | |
| // Acknowledge immediately: seeding the draft below can take 10s+ due to the | |
| // LLM round trip, and Discord drops unacknowledged interactions after 3 seconds. | |
| ctx.defer().await?; | |
| let temp = start_draft( | |
| ctx.serenity_context(), | |
| data, | |
| guild_id, | |
| author, | |
| None, | |
| Some(message.content.clone()), | |
| Some(original), | |
| ) | |
| .await?; |
| } | ||
| }; | ||
|
|
||
| post_issue(ctx, data, guild_id, &issue).await?; |
There was a problem hiding this comment.
🟡 MEDIUM · Correctness
confirm_draft removes the draft state and confirms it in the backend, then calls post_issue with ?. If posting fails (ISSUE_CHANNEL_ID unconfigured, Discord error), the error propagates out of the event handler with no interaction ack: the issue already exists in the backend, the draft state is gone so the user cannot retry, and the temp channel is never closed. Handle the failure and inform the user instead of dropping the flow silently.
| post_issue(ctx, data, guild_id, &issue).await?; | |
| if let Err(e) = post_issue(ctx, data, guild_id, &issue).await { | |
| // The issue already exists in the backend; surface the failure so the user | |
| // can post it manually instead of losing the flow silently. | |
| ack( | |
| ctx, | |
| mci, | |
| &format!( | |
| "Issue #{} was created but could not be posted to the issue channel: {e}", | |
| issue.id | |
| ), | |
| ) | |
| .await?; | |
| return Ok(()); | |
| } |
| impl IntoResponse for AppError { | ||
| fn into_response(self) -> Response { | ||
| match self { | ||
| AppError::Db(e) => (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()).into_response(), |
There was a problem hiding this comment.
🔵 LOW · Security
Database errors are serialized straight into the HTTP response body, leaking connection/query internals (and with no auth on the API, to anyone who can reach the port). Log the detail server-side and return a generic message to the client.
| AppError::Db(e) => (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()).into_response(), | |
| AppError::Db(e) => { | |
| eprintln!("db error: {e}"); | |
| (StatusCode::INTERNAL_SERVER_ERROR, "internal server error").into_response() | |
| } |
|
/attacca review |
|
⏳ Still working on this — taking longer than usual, please wait... |
1 similar comment
|
⏳ Still working on this — taking longer than usual, please wait... |
|
✅ Review posted below. |
Attacca Code ReviewReason: Attacca could not reach the AI service at its configured address. Re-run with |
There was a problem hiding this comment.
Attacca Code Review
This PR replaces the SeaORM scaffolding with a working two-service system: an axum + sqlx backend (issues, zakonim, Discord message mirroring, AI summary drafts via Anthropic/OpenAI-compatible APIs) and a poise/serenity Discord bot (slash commands, context-menu issue creation, per-user draft channels, Redis author cache), plus Docker/compose deployment. The overall structure is sound and transactions are used carefully, but there are five significant problems: the backend has zero authentication while compose publishes it (and Postgres with the shipped default credentials) on the host; the message-mirror upsert can overwrite stored content with NULL on content-less edit events; the draft-summary handler persists the user message before the LLM call so a failure breaks retries (consecutive 'user' roles are rejected by Anthropic); the context-menu/modal flows never defer before a multi-second LLM round trip, so Discord drops the interaction; and the confirm flow loses the in-memory draft state and leaves the interaction unanswered if posting the issue to the channel fails.
Posted 5 finding(s) inline below.
| environment: | ||
| BIND_ADDR: 0.0.0.0:80 | ||
| ports: | ||
| - "8080:80" |
There was a problem hiding this comment.
🔴 CRITICAL · Security
Every backend route (create/edit/close issues, comments, the message mirror, draft summaries) is unauthenticated — the bot calls carry no token and the author fields are fully client-supplied — and compose publishes the backend on the host's 0.0.0.0:8080. Anyone who can reach the host can create/close issues, spoof authors, tamper with the message mirror, and burn LLM API credits through /summary/drafts/*. Worse, the db service publishes 5432 with the shipped default credentials from .env.example (zako/zako), giving direct DB access. At minimum bind to loopback; ideally add a shared-secret auth layer (e.g. axum middleware checking an API_TOKEN env var, sent by the bot) and rotate the DB password.
| - "8080:80" | |
| - "127.0.0.1:8080:80" |
| ON CONFLICT (message_id) DO UPDATE SET \ | ||
| content = EXCLUDED.content, \ | ||
| edited_at = EXCLUDED.edited_at \ |
There was a problem hiding this comment.
🟠 HIGH · Correctness
The upsert blindly copies EXCLUDED.content into the existing row. The bot's propagate_update sends content: event.content (serenity's Option), which is None whenever a MESSAGE_UPDATE doesn't carry new text (embed/attachment/sticker edits are the common case). Binding None binds SQL NULL, so those edit events permanently wipe the stored content of the mirrored message. Guard the update with COALESCE so a content-less edit keeps the previously stored text.
| ON CONFLICT (message_id) DO UPDATE SET \ | |
| content = EXCLUDED.content, \ | |
| edited_at = EXCLUDED.edited_at \ | |
| ON CONFLICT (message_id) DO UPDATE SET \ | |
| content = COALESCE(EXCLUDED.content, message_events.content), \ | |
| edited_at = EXCLUDED.edited_at \ |
| sqlx::query( | ||
| "INSERT INTO summary_draft_messages (draft_id, role, content) VALUES ($1, 'user', $2)", | ||
| ) | ||
| .bind(id) | ||
| .bind(&req.content) | ||
| .execute(&state.pool) | ||
| .await?; | ||
|
|
||
| let history = sqlx::query_as::<_, DraftMessage>( | ||
| "SELECT role, content FROM summary_draft_messages WHERE draft_id = $1 ORDER BY id ASC", | ||
| ) | ||
| .bind(id) | ||
| .fetch_all(&state.pool) | ||
| .await?; | ||
| let transcript: Vec<(String, String)> = | ||
| history.into_iter().map(|m| (m.role, m.content)).collect(); | ||
|
|
||
| let reply = llm::reply(&state.http, &state.llm, &transcript).await?; | ||
|
|
||
| let mut tx = state.pool.begin().await?; | ||
| sqlx::query( | ||
| "INSERT INTO summary_draft_messages (draft_id, role, content) VALUES ($1, 'assistant', $2)", | ||
| ) | ||
| .bind(id) | ||
| .bind(&reply) | ||
| .execute(&mut *tx) | ||
| .await?; | ||
| sqlx::query("UPDATE summary_drafts SET summary = $2 WHERE id = $1") | ||
| .bind(id) | ||
| .bind(&reply) | ||
| .execute(&mut *tx) | ||
| .await?; |
There was a problem hiding this comment.
🟠 HIGH · Correctness
The user message is persisted before the LLM call. If the call fails (502 from a transient upstream error), the 'user' row stays in summary_draft_messages; the next attempt then sends two consecutive 'user' messages, which the Anthropic Messages API rejects (roles must alternate), so the draft is permanently stuck and every retry fails. The start_draft seeding path hits this too (a failed seed leaves [user] with no assistant reply). Build the transcript in memory, call the LLM first, and only persist user+assistant together in the transaction after success.
| sqlx::query( | |
| "INSERT INTO summary_draft_messages (draft_id, role, content) VALUES ($1, 'user', $2)", | |
| ) | |
| .bind(id) | |
| .bind(&req.content) | |
| .execute(&state.pool) | |
| .await?; | |
| let history = sqlx::query_as::<_, DraftMessage>( | |
| "SELECT role, content FROM summary_draft_messages WHERE draft_id = $1 ORDER BY id ASC", | |
| ) | |
| .bind(id) | |
| .fetch_all(&state.pool) | |
| .await?; | |
| let transcript: Vec<(String, String)> = | |
| history.into_iter().map(|m| (m.role, m.content)).collect(); | |
| let reply = llm::reply(&state.http, &state.llm, &transcript).await?; | |
| let mut tx = state.pool.begin().await?; | |
| sqlx::query( | |
| "INSERT INTO summary_draft_messages (draft_id, role, content) VALUES ($1, 'assistant', $2)", | |
| ) | |
| .bind(id) | |
| .bind(&reply) | |
| .execute(&mut *tx) | |
| .await?; | |
| sqlx::query("UPDATE summary_drafts SET summary = $2 WHERE id = $1") | |
| .bind(id) | |
| .bind(&reply) | |
| .execute(&mut *tx) | |
| .await?; | |
| let history = sqlx::query_as::<_, DraftMessage>( | |
| "SELECT role, content FROM summary_draft_messages WHERE draft_id = $1 ORDER BY id ASC", | |
| ) | |
| .bind(id) | |
| .fetch_all(&state.pool) | |
| .await?; | |
| let mut transcript: Vec<(String, String)> = | |
| history.into_iter().map(|m| (m.role, m.content)).collect(); | |
| // Keep the new user message in memory until the LLM call succeeds: persisting it first | |
| // leaves an orphaned 'user' row on failure, so a retry sends two consecutive user | |
| // messages and Anthropic rejects the request (roles must alternate). | |
| transcript.push(("user".to_string(), req.content.clone())); | |
| let reply = llm::reply(&state.http, &state.llm, &transcript).await?; | |
| let mut tx = state.pool.begin().await?; | |
| sqlx::query( | |
| "INSERT INTO summary_draft_messages (draft_id, role, content) VALUES ($1, 'user', $2)", | |
| ) | |
| .bind(id) | |
| .bind(&req.content) | |
| .execute(&mut *tx) | |
| .await?; | |
| sqlx::query( | |
| "INSERT INTO summary_draft_messages (draft_id, role, content) VALUES ($1, 'assistant', $2)", | |
| ) | |
| .bind(id) | |
| .bind(&reply) | |
| .execute(&mut *tx) | |
| .await?; | |
| sqlx::query("UPDATE summary_drafts SET summary = $2 WHERE id = $1") | |
| .bind(id) | |
| .bind(&reply) | |
| .execute(&mut *tx) | |
| .await?; | |
| tx.commit().await?; |
| let temp = start_draft( | ||
| ctx.serenity_context(), | ||
| data, | ||
| guild_id, | ||
| author, | ||
| None, | ||
| Some(message.content.clone()), | ||
| Some(original), | ||
| ) | ||
| .await?; | ||
|
|
||
| ctx.send( | ||
| poise::CreateReply::default() | ||
| .ephemeral(true) | ||
| .content("Started an issue draft from that message.") | ||
| .components(open_link_row(guild_id, temp, "Open draft")), | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
🟠 HIGH · Correctness
Neither the context-menu command nor the modal path in new acknowledges the interaction before start_draft, which performs a backend round trip plus an LLM call that routinely takes >3 seconds. Discord drops interactions not answered within 3 seconds, so ctx.send fails with an interaction-expired error and the user never sees the draft link (the slash new modal path at lines 155-172 has the same problem after the modal is submitted). Call ctx.defer().await? immediately after the guild check here (and right after NewIssueModal::execute returns in new) so the long work happens against a deferred interaction.
| let temp = start_draft( | |
| ctx.serenity_context(), | |
| data, | |
| guild_id, | |
| author, | |
| None, | |
| Some(message.content.clone()), | |
| Some(original), | |
| ) | |
| .await?; | |
| ctx.send( | |
| poise::CreateReply::default() | |
| .ephemeral(true) | |
| .content("Started an issue draft from that message.") | |
| .components(open_link_row(guild_id, temp, "Open draft")), | |
| ) | |
| .await?; | |
| ctx.defer().await?; | |
| let temp = start_draft( | |
| ctx.serenity_context(), | |
| data, | |
| guild_id, | |
| author, | |
| None, | |
| Some(message.content.clone()), | |
| Some(original), | |
| ) | |
| .await?; | |
| ctx.send( | |
| poise::CreateReply::default() | |
| .ephemeral(true) | |
| .content("Started an issue draft from that message.") | |
| .components(open_link_row(guild_id, temp, "Open draft")), | |
| ) | |
| .await?; | |
| Ok(()) |
| } | ||
| }; | ||
|
|
||
| post_issue(ctx, data, guild_id, &issue).await?; |
There was a problem hiding this comment.
🟠 HIGH · Correctness
confirm_draft removed the draft state from data.drafts at line 140 and the backend already marked the draft Confirmed (creating the issue) before this call. If post_issue fails (ISSUE_CHANNEL_ID unset, Discord API/rate-limit error), the ? propagates: no ack is sent so the interaction times out, the in-memory state is gone (a second Confirm click reports "no longer active"), the temp channel is left open with dead buttons, and the original_reference is never recorded — even though the issue was actually created in the backend. Handle the error explicitly and inform the user of the created issue id instead of dropping the flow.
| post_issue(ctx, data, guild_id, &issue).await?; | |
| if let Err(e) = post_issue(ctx, data, guild_id, &issue).await { | |
| // The issue already exists in the backend; surface the failure instead of dropping | |
| // the draft state (already removed above) and leaving the interaction unanswered. | |
| ack( | |
| ctx, | |
| mci, | |
| &format!( | |
| "Issue #{} was created, but posting it to the issue channel failed: {e}", | |
| issue.id | |
| ), | |
| ) | |
| .await?; | |
| return Ok(()); | |
| } |
1 participant
No description provided.