From 441ea454fcdbfe8050aa4124739923ceec425e86 Mon Sep 17 00:00:00 2001 From: Gaya KACI <163902005+kacigaya@users.noreply.github.com> Date: Tue, 3 Nov 2026 00:26:11 +0000 Subject: [PATCH 1/2] feat: add review controls, agent usage, security digest - Retry failed or cancelled reviews and cancel queued or running ones from the dashboard. Admin only. A per-PR lock and the worker lease keep retries from racing pushes or overlapping a stopping run. - Explain failed and cancelled reviews in plain words. Keep the raw error under Error details. - Record agent time and the token usage that each CLI reports. Codex runs with --json. Show harness availability from the model list check. - Add an opt-in weekly security digest. It posts new, fixed, and overdue Dependabot alerts to an HTTPS chat webhook. Delivery connects only to public addresses and never exposes the URL. - Store findings before posting and mark them published with the GitHub review ID in one transaction. --- Cargo.lock | 2 + README.md | 5 +- crates/server/Cargo.toml | 2 + crates/server/src/agents.rs | 207 ++++- crates/server/src/api.rs | 134 ++- crates/server/src/db.rs | 376 +++++++- crates/server/src/digest.rs | 863 ++++++++++++++++++ crates/server/src/github.rs | 53 +- crates/server/src/main.rs | 1 + crates/server/src/models.rs | 20 +- crates/server/src/review.rs | 53 +- crates/server/src/tests.rs | 419 ++++++++- crates/server/src/vulnerabilities.rs | 2 +- crates/server/src/webhook.rs | 7 +- crates/server/src/worker.rs | 31 +- docs/api.md | 95 +- docs/configuration.md | 39 + docs/github-app.md | 2 + docs/security.md | 17 +- migrations/0004_usage_health_digest.sql | 24 + .../(dashboard)/installations/[id]/actions.ts | 9 + .../(dashboard)/installations/[id]/page.tsx | 24 +- .../installations/[id]/settings/page.tsx | 3 +- web/app/(dashboard)/reviews/[id]/actions.ts | 32 + web/app/(dashboard)/reviews/[id]/page.tsx | 113 ++- web/components/agent-health.tsx | 134 +++ web/components/review-controls.tsx | 51 ++ web/components/settings-form.tsx | 78 ++ web/lib/api-client.ts | 10 +- web/lib/api.ts | 2 +- web/lib/docs-content.tsx | 30 +- web/lib/failure.test.ts | 57 ++ web/lib/failure.ts | 164 ++++ web/lib/format.test.ts | 15 +- web/lib/format.ts | 9 + web/lib/types.ts | 34 + web/scripts/smoke.ts | 96 +- 37 files changed, 3053 insertions(+), 160 deletions(-) create mode 100644 crates/server/src/digest.rs create mode 100644 migrations/0004_usage_health_digest.sql create mode 100644 web/app/(dashboard)/reviews/[id]/actions.ts create mode 100644 web/components/agent-health.tsx create mode 100644 web/components/review-controls.tsx create mode 100644 web/lib/failure.test.ts create mode 100644 web/lib/failure.ts diff --git a/Cargo.lock b/Cargo.lock index c9d7731..0fe7234 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -427,11 +427,13 @@ dependencies = [ "sqlx", "thiserror", "tokio", + "tokio-rustls", "tokio-util", "tower", "tracing", "tracing-subscriber", "uuid", + "webpki-roots", ] [[package]] diff --git a/README.md b/README.md index edacba9..868fade 100644 --- a/README.md +++ b/README.md @@ -49,7 +49,10 @@ A new push cancels the old review. A commit that already has a review is not rev Sign in with GitHub. Choose an installation to set its agent and model and to follow its review history. Open a review to read each finding and its -suggested fix. The screenshots use demo data. +suggested fix. Admins can retry failed reviews and cancel queued ones. The +overview shows agent health, run time, and token usage. An opt-in weekly +security digest posts Dependabot alert changes to a chat webhook. The +screenshots use demo data.

diff --git a/crates/server/Cargo.toml b/crates/server/Cargo.toml index 9510b6d..4cdfff1 100644 --- a/crates/server/Cargo.toml +++ b/crates/server/Cargo.toml @@ -26,10 +26,12 @@ sha2.workspace = true sqlx = { version = "0.9", default-features = false, features = ["runtime-tokio", "tls-rustls", "postgres", "macros", "migrate", "uuid", "chrono", "json"] } thiserror.workspace = true tokio = { version = "1", features = ["rt-multi-thread", "macros", "process", "signal", "time", "fs", "io-util", "sync"] } +tokio-rustls = { version = "0.26", default-features = false, features = ["ring", "tls12"] } tokio-util = "0.7" tracing = "0.1" tracing-subscriber = { version = "0.3", features = ["env-filter", "json"] } uuid = { version = "1", features = ["v4", "serde"] } +webpki-roots = "1" [dev-dependencies] tower = { version = "0.5", features = ["util"] } diff --git a/crates/server/src/agents.rs b/crates/server/src/agents.rs index cacbf15..a339691 100644 --- a/crates/server/src/agents.rs +++ b/crates/server/src/agents.rs @@ -165,6 +165,8 @@ pub fn command( "read-only", "--skip-git-repo-check", "--ephemeral", + // JSONL events on stdout carry token usage and errors. + "--json", "--ignore-user-config", "--ignore-rules", "--color", @@ -239,9 +241,13 @@ pub fn parse_output(agent: Agent, stdout: &str, result_file: Option<&str>) -> Re } } Agent::Codex => { - let text = result_file - .filter(|t| !t.trim().is_empty()) - .ok_or_else(|| anyhow!("codex wrote no final message"))?; + let text = + result_file + .filter(|t| !t.trim().is_empty()) + .ok_or_else(|| match codex_error(stdout) { + Some(message) => anyhow!("codex error: {}", short(&message)), + None => anyhow!("codex wrote no final message"), + })?; extract_json_object(text).context("codex final message has no JSON object")? } Agent::OpenCode => { @@ -273,6 +279,138 @@ pub fn parse_output(agent: Agent, stdout: &str, result_file: Option<&str>) -> Re serde_json::from_value(value).context("agent JSON does not match the review schema") } +/// The last error message in Codex JSONL events. +fn codex_error(stdout: &str) -> Option { + stdout + .lines() + .filter_map(|line| serde_json::from_str::(line).ok()) + .filter_map(|event| match event["type"].as_str()? { + "error" => event["message"].as_str().map(str::to_owned), + "turn.failed" => event.pointer("/error/message")?.as_str().map(str::to_owned), + _ => None, + }) + .next_back() +} + +/// Token usage that an agent CLI reports. Input counts include cached input. +#[derive(Debug, Default, Clone, PartialEq, Eq)] +pub struct Usage { + pub input_tokens: Option, + pub cached_input_tokens: Option, + pub output_tokens: Option, + /// The model that used the most tokens, when the CLI names it. + pub model: Option, +} + +impl Usage { + fn sum_opt(a: Option, b: Option) -> Option { + match (a, b) { + (None, None) => None, + (a, b) => Some(a.unwrap_or(0).saturating_add(b.unwrap_or(0))), + } + } + + /// Adds another run, such as a fallback agent run. + pub fn add(&mut self, other: Usage) { + self.input_tokens = Self::sum_opt(self.input_tokens, other.input_tokens); + self.cached_input_tokens = + Self::sum_opt(self.cached_input_tokens, other.cached_input_tokens); + self.output_tokens = Self::sum_opt(self.output_tokens, other.output_tokens); + if other.model.is_some() { + self.model = other.model; + } + } +} + +fn tokens(value: &Value, pointer: &str) -> Option { + value.pointer(pointer)?.as_i64().filter(|n| *n >= 0) +} + +/// Reads token usage from agent stdout. Fields the CLI does not report stay +/// `None`. Formats: +/// - Claude: `modelUsage` per model, or `usage`, in the result envelope. +/// - Codex: `usage` of each `turn.completed` event. Input includes cached input. +/// - OpenCode: `part.tokens` of each `step_finish` event. +pub fn parse_usage(agent: Agent, stdout: &str) -> Usage { + let mut usage = Usage::default(); + match agent { + Agent::Claude => { + let Ok(envelope) = serde_json::from_str::(stdout.trim()) else { + return usage; + }; + if let Some(models) = envelope["modelUsage"].as_object().filter(|m| !m.is_empty()) { + let mut largest = 0; + for (model, m) in models { + let cached = tokens(m, "/cacheReadInputTokens"); + let input = [ + tokens(m, "/inputTokens"), + tokens(m, "/cacheCreationInputTokens"), + ] + .into_iter() + .fold(cached, Usage::sum_opt); + let output = tokens(m, "/outputTokens"); + let total = input.unwrap_or(0) + output.unwrap_or(0); + usage.add(Usage { + input_tokens: input, + cached_input_tokens: cached, + output_tokens: output, + model: None, + }); + if total >= largest && valid_model(model) { + largest = total; + usage.model = Some(model.clone()); + } + } + } else if envelope["usage"].is_object() { + let cached = tokens(&envelope, "/usage/cache_read_input_tokens"); + usage.input_tokens = [ + tokens(&envelope, "/usage/input_tokens"), + tokens(&envelope, "/usage/cache_creation_input_tokens"), + ] + .into_iter() + .fold(cached, Usage::sum_opt); + usage.cached_input_tokens = cached; + usage.output_tokens = tokens(&envelope, "/usage/output_tokens"); + } + } + Agent::Codex | Agent::OpenCode => { + for event in stdout + .lines() + .filter_map(|line| serde_json::from_str::(line).ok()) + { + let run = match (agent, event["type"].as_str()) { + (Agent::Codex, Some("turn.completed")) => Usage { + input_tokens: tokens(&event, "/usage/input_tokens"), + cached_input_tokens: tokens(&event, "/usage/cached_input_tokens"), + output_tokens: tokens(&event, "/usage/output_tokens"), + model: None, + }, + (Agent::OpenCode, Some("step_finish")) => { + let cached = tokens(&event, "/part/tokens/cache/read"); + Usage { + input_tokens: [ + tokens(&event, "/part/tokens/input"), + tokens(&event, "/part/tokens/cache/write"), + ] + .into_iter() + .fold(cached, Usage::sum_opt), + cached_input_tokens: cached, + output_tokens: Usage::sum_opt( + tokens(&event, "/part/tokens/output"), + tokens(&event, "/part/tokens/reasoning"), + ), + model: None, + } + } + _ => continue, + }; + usage.add(run); + } + } + } + usage +} + fn short(text: &str) -> String { text.chars().take(300).collect() } @@ -306,6 +444,7 @@ mod tests { let (codex, _) = command(Agent::Codex, None, &paths(), false); assert!(codex.windows(2).any(|w| w == ["--sandbox", "read-only"])); + assert!(codex.iter().any(|arg| arg == "--json")); assert_eq!(codex.last().unwrap(), "-"); let (opencode, env) = command(Agent::OpenCode, None, &paths(), false); @@ -387,6 +526,68 @@ mod tests { assert!(parse_output(Agent::Codex, "", None).is_err()); } + #[test] + fn usage_from_each_cli() { + // Shapes from Claude Code 2.1.310 and Codex 0.166.0 output on 2026-11-02. + let claude = r#"{"type":"result","usage":{"input_tokens":2,"cache_creation_input_tokens":1250,"cache_read_input_tokens":1670,"output_tokens":4}, + "modelUsage":{"claude-sonnet-5-5":{"inputTokens":2,"outputTokens":4,"cacheReadInputTokens":1670,"cacheCreationInputTokens":1250}, + "claude-haiku-5-5":{"inputTokens":10,"outputTokens":1}}}"#; + assert_eq!( + parse_usage(Agent::Claude, claude), + Usage { + input_tokens: Some(2932), + cached_input_tokens: Some(1670), + output_tokens: Some(5), + model: Some("claude-sonnet-5-5".into()), + } + ); + let envelope_only = + r#"{"usage":{"input_tokens":5,"cache_read_input_tokens":3,"output_tokens":7}}"#; + assert_eq!( + parse_usage(Agent::Claude, envelope_only), + Usage { + input_tokens: Some(8), + cached_input_tokens: Some(3), + output_tokens: Some(7), + model: None, + } + ); + let codex = concat!( + r#"{"type":"thread.started","thread_id":"x"}"#, + "\n", + r#"{"type":"turn.completed","usage":{"input_tokens":24388,"cached_input_tokens":12288,"output_tokens":5}}"#, + "\n", + r#"{"type":"turn.completed","usage":{"input_tokens":12,"cached_input_tokens":0,"output_tokens":1}}"#, + ); + let usage = parse_usage(Agent::Codex, codex); + assert_eq!(usage.input_tokens, Some(24400)); + assert_eq!(usage.cached_input_tokens, Some(12288)); + assert_eq!(usage.output_tokens, Some(6)); + let opencode = r#"{"type":"step_finish","part":{"type":"step-finish","tokens":{"input":10,"output":4,"reasoning":2,"cache":{"read":30,"write":5}}}}"#; + assert_eq!( + parse_usage(Agent::OpenCode, opencode), + Usage { + input_tokens: Some(45), + cached_input_tokens: Some(30), + output_tokens: Some(6), + model: None, + } + ); + for agent in [Agent::Claude, Agent::Codex, Agent::OpenCode] { + assert_eq!(parse_usage(agent, "not json"), Usage::default()); + } + let mut total = Usage::default(); + total.add(Usage::default()); + assert_eq!(total, Usage::default()); + } + + #[test] + fn codex_errors_come_from_json_events() { + let out = r#"{"type":"turn.failed","error":{"message":"You've hit your usage limit"}}"#; + let err = parse_output(Agent::Codex, out, None).unwrap_err(); + assert!(err.to_string().contains("usage limit"), "{err}"); + } + #[test] fn parses_opencode_events() { let out = concat!( diff --git a/crates/server/src/api.rs b/crates/server/src/api.rs index ed51719..5cce99b 100644 --- a/crates/server/src/api.rs +++ b/crates/server/src/api.rs @@ -12,7 +12,7 @@ use axum::extract::{FromRequestParts, Path, Query, State}; use axum::http::StatusCode; use axum::http::request::Parts; use axum::response::{IntoResponse, Response}; -use axum::routing::{get, patch}; +use axum::routing::{get, patch, post}; use axum::{Json, Router}; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; @@ -39,6 +39,8 @@ pub fn router() -> Router { .route("/installations/{id}/vulnerabilities", get(vulnerabilities)) .route("/reviews/{id}", get(get_review)) .route("/reviews/{id}/pull", get(review_pull)) + .route("/reviews/{id}/retry", post(retry_review)) + .route("/reviews/{id}/cancel", post(cancel_review)) } pub struct ApiError(StatusCode, String); @@ -200,22 +202,34 @@ async fn me(access: Access) -> ApiResult { /// Needs a token: OpenCode model lists show which providers have keys. async fn agents(State(state): State, _: Access) -> ApiResult { let names: Vec<&str> = state.settings.agents.iter().map(|a| a.name()).collect(); - // Only enabled agents. An agent without a stored list maps to `[]`. - let stored: HashMap = - db::agent_models(&state.db).await?.into_iter().collect(); - let models: serde_json::Map = names - .iter() - .map(|name| { - ( - name.to_string(), - stored.get(*name).cloned().unwrap_or(json!([])), - ) - }) + // Only enabled agents. An agent without a stored list maps to `[]`, and + // to `null` health until its first check. + let stored: HashMap = db::agent_models(&state.db) + .await? + .into_iter() + .map(|row| (row.agent.clone(), row)) .collect(); + let mut models = serde_json::Map::new(); + let mut health = serde_json::Map::new(); + for name in &names { + let row = stored.get(*name); + models.insert( + name.to_string(), + row.map_or(json!([]), |r| r.models.clone()), + ); + health.insert( + name.to_string(), + row.map_or( + serde_json::Value::Null, + |r| json!({ "available": r.available, "checked_at": r.checked_at }), + ), + ); + } Ok(Json(json!({ "agents": names, "default": state.settings.default_agent.name(), "models": models, + "health": health, }))) } @@ -230,6 +244,7 @@ struct InstallationView { suspended: bool, created_at: DateTime, settings: SettingsView, + digest: DigestView, can_admin: bool, } @@ -240,6 +255,16 @@ struct SettingsView { model: Option, } +/// The webhook URL is a secret. Only its host is shown. +#[derive(Serialize)] +struct DigestView { + enabled: bool, + webhook_host: Option, + sent_at: Option>, + attempted_at: Option>, + error: Option, +} + fn view(i: db::Installation, can_admin: bool) -> InstallationView { InstallationView { id: i.id, @@ -254,6 +279,16 @@ fn view(i: db::Installation, can_admin: bool) -> InstallationView { agent: i.agent, model: i.model, }, + digest: DigestView { + enabled: i.digest_webhook_url.is_some(), + webhook_host: i + .digest_webhook_url + .as_deref() + .and_then(crate::digest::webhook_host), + sent_at: i.digest_sent_at, + attempted_at: i.digest_attempted_at, + error: i.digest_error, + }, can_admin, } } @@ -312,6 +347,9 @@ struct SettingsPatch { agent: Option>, #[serde(default, deserialize_with = "double_option")] model: Option>, + /// `null` turns the weekly security digest off. + #[serde(default, deserialize_with = "double_option")] + digest_webhook_url: Option>, } async fn update_settings( @@ -350,8 +388,24 @@ async fn update_settings( Some(None) => None, None => current.model, }; + let digest = match &patch.digest_webhook_url { + Some(Some(url)) => { + crate::digest::validate_webhook_url(url).map_err(unprocessable)?; + Some(Some(url.as_str())) + } + Some(None) => Some(None), + None => None, + }; let enabled = patch.enabled.unwrap_or(current.enabled); - db::update_settings(&state.db, id, enabled, agent.as_deref(), model.as_deref()).await?; + db::update_settings( + &state.db, + id, + enabled, + agent.as_deref(), + model.as_deref(), + digest, + ) + .await?; tracing::info!(installation = id, user = %access.user.login, "settings updated"); let (installation, can_admin) = load_installation(&state, &access, id).await?; Ok(Json(view(installation, can_admin))) @@ -580,3 +634,57 @@ async fn vulnerabilities( .map_err(github_error)?; Ok(Json(report)) } + +/// Loads a review that the user can see and whose installation they administer. +async fn admin_review(state: &AppState, access: &Access, id: Uuid) -> Result<(), ApiError> { + let not_found = || ApiError::new(StatusCode::NOT_FOUND, "review not found"); + let (review, _) = db::review(&state.db, id).await?.ok_or_else(not_found)?; + match access.installations.get(&review.installation_id) { + None => Err(not_found()), + Some(false) => Err(ApiError::new( + StatusCode::FORBIDDEN, + "admin access required", + )), + Some(true) => Ok(()), + } +} + +async fn updated_review(state: &AppState, id: Uuid) -> ApiResult { + let (review, _) = db::review(&state.db, id) + .await? + .ok_or_else(|| ApiError::new(StatusCode::NOT_FOUND, "review not found"))?; + Ok(Json(review)) +} + +async fn retry_review( + State(state): State, + access: Access, + Path(id): Path, +) -> ApiResult { + admin_review(&state, &access, id).await?; + if !db::requeue_review(&state.db, id).await? { + return Err(ApiError::new( + StatusCode::CONFLICT, + "only the newest failed or cancelled review of an idle pull request can be retried; \ + a cancelled agent may still be stopping, so try again in a minute", + )); + } + tracing::info!(review = %id, user = %access.user.login, "review retried"); + updated_review(&state, id).await +} + +async fn cancel_review( + State(state): State, + access: Access, + Path(id): Path, +) -> ApiResult { + admin_review(&state, &access, id).await?; + if !db::cancel_review(&state.db, id).await? { + return Err(ApiError::new( + StatusCode::CONFLICT, + "only queued or running reviews can be cancelled", + )); + } + tracing::info!(review = %id, user = %access.user.login, "review cancelled"); + updated_review(&state, id).await +} diff --git a/crates/server/src/db.rs b/crates/server/src/db.rs index 5945428..c106d28 100644 --- a/crates/server/src/db.rs +++ b/crates/server/src/db.rs @@ -61,8 +61,18 @@ pub struct Installation { pub agent: Option, pub model: Option, pub created_at: DateTime, + /// Secret. Never return it from the API. + pub digest_webhook_url: Option, + pub digest_sent_at: Option>, + pub digest_attempted_at: Option>, + pub digest_error: Option, } +const INSTALLATION_COLUMNS: &str = + "id, account_id, account_login, account_type, repository_selection, + suspended, enabled, agent, model, created_at, + digest_webhook_url, digest_sent_at, digest_attempted_at, digest_error"; + pub struct InstallationUpsert<'a> { pub id: i64, pub account_id: i64, @@ -119,41 +129,53 @@ pub async fn set_installation_state( } pub async fn installation(db: &Db, id: i64) -> sqlx::Result> { - sqlx::query_as( - "SELECT id, account_id, account_login, account_type, repository_selection, suspended, - enabled, agent, model, created_at - FROM installations WHERE id = $1 AND NOT deleted", - ) - .bind(id) - .fetch_optional(db) - .await + let sql = + format!("SELECT {INSTALLATION_COLUMNS} FROM installations WHERE id = $1 AND NOT deleted"); + sqlx::query_as(sqlx::AssertSqlSafe(sql)) + .bind(id) + .fetch_optional(db) + .await } pub async fn installations(db: &Db, ids: &[i64]) -> sqlx::Result> { - sqlx::query_as( - "SELECT id, account_id, account_login, account_type, repository_selection, suspended, - enabled, agent, model, created_at - FROM installations WHERE id = ANY($1) AND NOT deleted ORDER BY account_login", - ) - .bind(ids) - .fetch_all(db) - .await + let sql = format!( + "SELECT {INSTALLATION_COLUMNS} FROM installations + WHERE id = ANY($1) AND NOT deleted ORDER BY account_login" + ); + sqlx::query_as(sqlx::AssertSqlSafe(sql)) + .bind(ids) + .fetch_all(db) + .await } +/// `digest_webhook_url`: `None` keeps the current value, `Some(None)` turns +/// the digest off. A new URL clears the last result and sends soon. pub async fn update_settings( db: &Db, id: i64, enabled: bool, agent: Option<&str>, model: Option<&str>, + digest_webhook_url: Option>, ) -> sqlx::Result<()> { sqlx::query( - "UPDATE installations SET enabled = $2, agent = $3, model = $4, updated_at = now() WHERE id = $1", + "UPDATE installations SET enabled = $2, agent = $3, model = $4, + digest_webhook_url = CASE WHEN $5 THEN $6 ELSE digest_webhook_url END, + digest_attempted_at = CASE WHEN $5 AND $6 IS DISTINCT FROM digest_webhook_url + THEN NULL ELSE digest_attempted_at END, + digest_error = CASE WHEN $5 AND $6 IS DISTINCT FROM digest_webhook_url + THEN NULL ELSE digest_error END, + digest_sent_at = CASE WHEN $5 AND $6 IS DISTINCT FROM digest_webhook_url + THEN NULL ELSE digest_sent_at END, + updated_at = now() + WHERE id = $1", ) .bind(id) .bind(enabled) .bind(agent) .bind(model) + .bind(digest_webhook_url.is_some()) + .bind(digest_webhook_url.flatten()) .execute(db) .await?; Ok(()) @@ -185,7 +207,15 @@ pub struct NewReview<'a> { /// reviews of the same pull request are cancelled. pub async fn enqueue_review(db: &Db, r: &NewReview<'_>) -> sqlx::Result> { let mut tx = db.begin().await?; - cancel_pr_reviews(&mut *tx, r.repo_id, r.pr_number, Some(r.head_sha)).await?; + lock_pull(&mut tx, r.repo_id, r.pr_number).await?; + cancel_pr_reviews( + &mut *tx, + r.repo_id, + r.pr_number, + Some(r.head_sha), + "superseded", + ) + .await?; let id: Option = sqlx::query_scalar( "INSERT INTO reviews (id, installation_id, repo_id, repo_full_name, pr_number, pr_title, head_sha, base_sha) VALUES ($1, $2, $3, $4, $5, $6, $7, $8) @@ -211,15 +241,17 @@ pub async fn cancel_pr_reviews<'e>( repo_id: i64, pr_number: i64, keep_sha: Option<&str>, + reason: &str, ) -> sqlx::Result { let result = sqlx::query( - "UPDATE reviews SET status = 'cancelled', finished_at = now(), error = 'superseded' + "UPDATE reviews SET status = 'cancelled', finished_at = now(), error = $4 WHERE repo_id = $1 AND pr_number = $2 AND status IN ('queued', 'running') AND ($3::text IS NULL OR head_sha <> $3)", ) .bind(repo_id) .bind(pr_number as i32) .bind(keep_sha) + .bind(reason) .execute(executor) .await?; Ok(result.rows_affected()) @@ -292,6 +324,16 @@ pub async fn requeue_stale(db: &Db, older_than: Duration) -> sqlx::Result { .bind(older_than.as_secs_f64()) .execute(db) .await?; + // A worker that died after another writer finished its review never + // releases the lease. Drop it so the review can be retried. + sqlx::query( + "UPDATE reviews SET locked_by = NULL + WHERE status <> 'running' AND locked_by IS NOT NULL + AND (heartbeat_at IS NULL OR heartbeat_at < now() - make_interval(secs => $1))", + ) + .bind(older_than.as_secs_f64()) + .execute(db) + .await?; Ok(result.rows_affected()) } @@ -315,12 +357,18 @@ pub async fn prune_reviews(db: &Db) -> sqlx::Result { Ok(result.rows_affected()) } +#[derive(Default)] pub struct Finish<'a> { pub status: &'a str, pub summary: Option<&'a str>, pub error: Option<&'a str>, pub agent: Option<&'a str>, pub model: Option<&'a str>, + /// Time in agent processes. Null when no agent ran. + pub agent_ms: Option, + pub input_tokens: Option, + pub cached_input_tokens: Option, + pub output_tokens: Option, } /// Sets a final state. Has no effect when the review is no longer ours. @@ -328,7 +376,8 @@ pub async fn finish_review(db: &Db, id: Uuid, worker: &str, f: &Finish<'_>) -> s sqlx::query( "UPDATE reviews SET status = $3, summary = $4, error = $5, agent = COALESCE($6, agent), model = COALESCE($7, model), - finished_at = now(), locked_by = NULL + agent_duration_ms = $8, input_tokens = $9, cached_input_tokens = $10, + output_tokens = $11, finished_at = now(), locked_by = NULL WHERE id = $1 AND status = 'running' AND locked_by = $2", ) .bind(id) @@ -338,6 +387,10 @@ pub async fn finish_review(db: &Db, id: Uuid, worker: &str, f: &Finish<'_>) -> s .bind(f.error) .bind(f.agent) .bind(f.model) + .bind(f.agent_ms) + .bind(f.input_tokens) + .bind(f.cached_input_tokens) + .bind(f.output_tokens) .execute(db) .await?; Ok(()) @@ -377,30 +430,165 @@ pub async fn release_review(db: &Db, id: Uuid, worker: &str) -> sqlx::Result<()> Ok(()) } -pub async fn set_github_review_id(db: &Db, id: Uuid, review_id: i64) -> sqlx::Result<()> { - sqlx::query("UPDATE reviews SET github_review_id = $2 WHERE id = $1") - .bind(id) - .bind(review_id) - .execute(db) +/// Records the agent before it runs, so running and failed reviews show it. +pub async fn set_review_agent( + db: &Db, + id: Uuid, + worker: &str, + agent: &str, + model: Option<&str>, +) -> sqlx::Result<()> { + sqlx::query( + "UPDATE reviews SET agent = $3, model = $4 + WHERE id = $1 AND status = 'running' AND locked_by = $2", + ) + .bind(id) + .bind(worker) + .bind(agent) + .bind(model) + .execute(db) + .await?; + Ok(()) +} + +/// Cancels a queued or running review. A running worker sees the change at +/// its next heartbeat and stops the agent. The worker keeps its lease until it +/// has stopped, see [`release_lock`]. Returns false when the review was not +/// queued or running. +pub async fn cancel_review(db: &Db, id: Uuid) -> sqlx::Result { + let result = sqlx::query( + "UPDATE reviews SET status = 'cancelled', finished_at = now(), + error = 'cancelled from the dashboard' + WHERE id = $1 AND status IN ('queued', 'running')", + ) + .bind(id) + .execute(db) + .await?; + Ok(result.rows_affected() == 1) +} + +/// Drops the lease of a review that another writer finished, after the worker +/// has stopped its agent. +pub async fn release_lock(db: &Db, id: Uuid, worker: &str) -> sqlx::Result<()> { + sqlx::query( + "UPDATE reviews SET locked_by = NULL + WHERE id = $1 AND locked_by = $2 AND status <> 'running'", + ) + .bind(id) + .bind(worker) + .execute(db) + .await?; + Ok(()) +} + +/// Queues a failed or cancelled review again with fresh attempts. Only the +/// newest review of a pull request qualifies, and only while no other review +/// of it is active, so an old commit never replaces a newer one. A review +/// whose worker still holds the lease waits, so two runs never share a work +/// directory or container name. The GitHub +/// review ID stays, so a review that was already posted is not posted twice. +/// Returns false when the review does not qualify. +pub async fn requeue_review(db: &Db, id: Uuid) -> sqlx::Result { + let mut tx = db.begin().await?; + let Some((repo_id, pr_number)): Option<(i64, i32)> = + sqlx::query_as("SELECT repo_id, pr_number FROM reviews WHERE id = $1") + .bind(id) + .fetch_optional(&mut *tx) + .await? + else { + return Ok(false); + }; + lock_pull(&mut tx, repo_id, pr_number.into()).await?; + let result = sqlx::query( + "UPDATE reviews r SET status = 'queued', attempts = 0, run_after = now(), + created_at = now(), started_at = NULL, finished_at = NULL, heartbeat_at = NULL, + locked_by = NULL, error = NULL, summary = NULL, agent = NULL, model = NULL, + agent_duration_ms = NULL, input_tokens = NULL, cached_input_tokens = NULL, + output_tokens = NULL + WHERE r.id = $1 AND r.status IN ('failed', 'cancelled') AND r.locked_by IS NULL + AND NOT EXISTS ( + SELECT FROM reviews x + WHERE x.repo_id = r.repo_id AND x.pr_number = r.pr_number AND x.id <> r.id + AND (x.status IN ('queued', 'running') OR x.created_at > r.created_at))", + ) + .bind(id) + .execute(&mut *tx) + .await?; + tx.commit().await?; + Ok(result.rows_affected() == 1) +} + +/// Serializes queue changes of one pull request until the transaction ends, +/// so a push and a dashboard retry cannot both queue a review. +async fn lock_pull( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + repo_id: i64, + pr_number: i64, +) -> sqlx::Result<()> { + sqlx::query("SELECT pg_advisory_xact_lock(hashtextextended($1, 0))") + .bind(format!("crocode:pull:{repo_id}:{pr_number}")) + .execute(&mut **tx) .await?; Ok(()) } +/// Fingerprints that other reviews of the pull request published. The rows of +/// `review_id` itself are left out: a retried review replaces them, and its +/// earlier attempt may already have posted them. pub async fn published_fingerprints( db: &Db, repo_id: i64, pr_number: i64, + review_id: Uuid, ) -> sqlx::Result> { let rows: Vec = sqlx::query_scalar( - "SELECT fingerprint FROM findings WHERE repo_id = $1 AND pr_number = $2 AND published", + "SELECT fingerprint FROM findings + WHERE repo_id = $1 AND pr_number = $2 AND published AND review_id <> $3", ) .bind(repo_id) .bind(pr_number as i32) + .bind(review_id) .fetch_all(db) .await?; Ok(rows.into_iter().collect()) } +/// Saves the GitHub review ID and marks the posted findings, together. +pub async fn mark_published( + db: &Db, + review_id: Uuid, + github_review_id: i64, + fingerprints: &[&str], +) -> sqlx::Result<()> { + let mut tx = db.begin().await?; + sqlx::query("UPDATE reviews SET github_review_id = $2 WHERE id = $1") + .bind(review_id) + .bind(github_review_id) + .execute(&mut *tx) + .await?; + sqlx::query( + "UPDATE findings SET published = TRUE WHERE review_id = $1 AND fingerprint = ANY($2)", + ) + .bind(review_id) + .bind(fingerprints) + .execute(&mut *tx) + .await?; + tx.commit().await +} + +/// Fingerprints that an earlier attempt of this review published. +pub async fn review_published_fingerprints( + db: &Db, + review_id: Uuid, +) -> sqlx::Result> { + let rows: Vec = + sqlx::query_scalar("SELECT fingerprint FROM findings WHERE review_id = $1 AND published") + .bind(review_id) + .fetch_all(db) + .await?; + Ok(rows.into_iter().collect()) +} + pub struct NewFinding<'a> { pub fingerprint: &'a str, pub path: &'a str, @@ -423,8 +611,10 @@ pub async fn insert_findings( files: &[(&str, &str)], ) -> sqlx::Result<()> { let mut tx = db.begin().await?; - // A retried review replaces the findings of its earlier attempt. - sqlx::query("DELETE FROM findings WHERE review_id = $1") + // A retried review replaces the unpublished findings of its earlier + // attempt. Published rows stay as the record of what GitHub received; + // callers skip their fingerprints. + sqlx::query("DELETE FROM findings WHERE review_id = $1 AND NOT published") .bind(review_id) .execute(&mut *tx) .await?; @@ -482,12 +672,17 @@ pub struct ReviewRow { pub error: Option, pub created_at: DateTime, pub finished_at: Option>, + pub agent_duration_ms: Option, + pub input_tokens: Option, + pub cached_input_tokens: Option, + pub output_tokens: Option, } const REVIEW_COLUMNS: &str = "r.id, r.installation_id, r.repo_full_name, r.pr_number, r.pr_title, r.head_sha, r.status, r.agent, r.model, r.attempts, (SELECT count(*) FROM findings f WHERE f.review_id = r.id) AS finding_count, - r.error, r.created_at, r.finished_at"; + r.error, r.created_at, r.finished_at, r.agent_duration_ms, r.input_tokens, + r.cached_input_tokens, r.output_tokens"; /// Reviews of one installation, newest first. `pull` limits the list to one /// pull request as `(repo_full_name, number)`. @@ -565,6 +760,22 @@ pub struct Day { pub failed: i64, } +/// Reviews of one agent. Token sums cover only reviews that reported usage. +#[derive(Debug, Serialize, sqlx::FromRow)] +pub struct AgentUsage { + pub name: String, + pub reviews: i64, + pub succeeded: i64, + pub failed: i64, + pub median_agent_secs: Option, + /// Reviews that reported token usage. + pub usage_reviews: i64, + pub input_tokens: i64, + pub cached_input_tokens: i64, + pub output_tokens: i64, + pub models: Vec, +} + #[derive(Debug, Serialize)] pub struct Stats { pub days: i32, @@ -577,6 +788,8 @@ pub struct Stats { pub categories: Vec, /// One entry per UTC day, oldest first. pub daily: Vec, + /// Reviews with a picked agent, most used first. + pub agents: Vec, } pub async fn stats(db: &Db, installation_id: i64) -> sqlx::Result { @@ -642,6 +855,27 @@ pub async fn stats(db: &Db, installation_id: i64) -> sqlx::Result { .bind(STATS_DAYS) .fetch_all(&mut *tx) .await?; + let agents: Vec = sqlx::query_as( + "SELECT agent AS name, count(*) AS reviews, + count(*) FILTER (WHERE status = 'succeeded') AS succeeded, + count(*) FILTER (WHERE status = 'failed') AS failed, + percentile_cont(0.5) WITHIN GROUP (ORDER BY agent_duration_ms / 1000.0) + FILTER (WHERE agent_duration_ms IS NOT NULL) AS median_agent_secs, + count(input_tokens) AS usage_reviews, + COALESCE(sum(input_tokens), 0)::int8 AS input_tokens, + COALESCE(sum(cached_input_tokens), 0)::int8 AS cached_input_tokens, + COALESCE(sum(output_tokens), 0)::int8 AS output_tokens, + COALESCE(array_agg(DISTINCT model) FILTER (WHERE model IS NOT NULL), '{}') + AS models + FROM reviews + WHERE installation_id = $1 AND agent IS NOT NULL + AND created_at >= now() - make_interval(days => $2) + GROUP BY agent ORDER BY count(*) DESC, agent", + ) + .bind(installation_id) + .bind(STATS_DAYS) + .fetch_all(&mut *tx) + .await?; tx.commit().await?; Ok(Stats { days: STATS_DAYS, @@ -651,13 +885,16 @@ pub async fn stats(db: &Db, installation_id: i64) -> sqlx::Result { severity, categories, daily, + agents, }) } pub async fn save_agent_models(db: &Db, agent: &str, models: &T) -> sqlx::Result<()> { sqlx::query( - "INSERT INTO agent_models (agent, models, detected_at) VALUES ($1, $2, now()) - ON CONFLICT (agent) DO UPDATE SET models = EXCLUDED.models, detected_at = now()", + "INSERT INTO agent_models (agent, models, available, detected_at, checked_at) + VALUES ($1, $2, TRUE, now(), now()) + ON CONFLICT (agent) DO UPDATE SET models = EXCLUDED.models, available = TRUE, + detected_at = now(), checked_at = now()", ) .bind(agent) .bind(sqlx::types::Json(models)) @@ -666,9 +903,31 @@ pub async fn save_agent_models(db: &Db, agent: &str, models: &T) - Ok(()) } -/// Stored model lists by agent name. -pub async fn agent_models(db: &Db) -> sqlx::Result> { - sqlx::query_as("SELECT agent, models FROM agent_models ORDER BY agent") +/// Records a failed model list run. Keeps the models of the last success. +pub async fn mark_agent_unavailable(db: &Db, agent: &str) -> sqlx::Result<()> { + sqlx::query( + "INSERT INTO agent_models (agent, models, available, checked_at) + VALUES ($1, '[]', FALSE, now()) + ON CONFLICT (agent) DO UPDATE SET available = FALSE, checked_at = now()", + ) + .bind(agent) + .execute(db) + .await?; + Ok(()) +} + +#[derive(Debug, sqlx::FromRow)] +pub struct AgentModels { + pub agent: String, + pub models: serde_json::Value, + /// False when the latest model list run failed. + pub available: bool, + pub checked_at: DateTime, +} + +/// Stored model lists and check results by agent name. +pub async fn agent_models(db: &Db) -> sqlx::Result> { + sqlx::query_as("SELECT agent, models, available, checked_at FROM agent_models ORDER BY agent") .fetch_all(db) .await } @@ -734,3 +993,52 @@ pub async fn review_contents( tx.commit().await?; Ok((findings, files)) } + +#[derive(Debug, sqlx::FromRow)] +pub struct DigestJob { + pub id: i64, + pub account_login: String, + pub digest_webhook_url: String, + pub digest_sent_at: Option>, +} + +/// Claims one installation whose weekly digest is due. A failed send is +/// tried again after an hour. Safe with several workers. +pub async fn claim_digest(db: &Db) -> sqlx::Result> { + sqlx::query_as( + "UPDATE installations SET digest_attempted_at = now() + WHERE id = ( + SELECT id FROM installations + WHERE digest_webhook_url IS NOT NULL AND NOT deleted AND NOT suspended + AND (digest_sent_at IS NULL OR digest_sent_at <= now() - interval '7 days') + AND (digest_attempted_at IS NULL OR digest_attempted_at <= now() - interval '1 hour') + ORDER BY digest_attempted_at NULLS FIRST + FOR UPDATE SKIP LOCKED + LIMIT 1) + RETURNING id, account_login, digest_webhook_url, digest_sent_at", + ) + .fetch_optional(db) + .await +} + +/// Records a digest result. `Ok` holds the start of the alert read, so the next +/// window starts there and misses no alert. Has no effect when an admin +/// changed the URL after the claim. +pub async fn finish_digest( + db: &Db, + job: &DigestJob, + result: Result, &str>, +) -> sqlx::Result<()> { + sqlx::query( + "UPDATE installations SET digest_error = $3, + digest_sent_at = COALESCE($4, digest_sent_at) + WHERE id = $1 AND digest_webhook_url = $2", + ) + .bind(job.id) + .bind(&job.digest_webhook_url) + .bind(result.err()) + .bind(result.ok()) + .execute(db) + .await?; + Ok(()) +} diff --git a/crates/server/src/digest.rs b/crates/server/src/digest.rs new file mode 100644 index 0000000..79ea7ad --- /dev/null +++ b/crates/server/src/digest.rs @@ -0,0 +1,863 @@ +//! Opt-in weekly security digest. +//! +//! An installation admin sets an HTTPS webhook URL. Every 7 days the worker +//! reads the Dependabot alerts of the installation with a read-only token and +//! posts a Slack-compatible message (`{"text": ...}`) to that URL. +//! +//! The URL is a secret and can point anywhere, so the worker only connects to +//! public addresses. It resolves the host once, rejects private, loopback, +//! link-local, and other special addresses, and connects to the checked +//! address. It sends one request and follows no redirects. Error messages +//! never contain the URL. + +use std::net::{IpAddr, Ipv4Addr, SocketAddr}; +use std::sync::Arc; +use std::time::Duration; + +use anyhow::{Context, Result}; +use axum::http::Uri; +use chrono::{DateTime, Utc}; +use octocrab::{Octocrab, Page}; +use serde::Deserialize; +use serde::de::DeserializeOwned; +use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt}; +use tokio::task::JoinSet; +use tokio_rustls::rustls; +use tokio_util::sync::CancellationToken; +use tracing::{info, warn}; + +use crate::github::valid_repo_name; +use crate::vulnerabilities::{Advisory, CoverageStatus, Dependency, Vulnerability}; +use crate::{AppState, db}; + +/// How often the worker looks for due digests. +const POLL: Duration = Duration::from_secs(10 * 60); +const PERIOD_DAYS: i64 = 7; +/// Longest window after failed sends. +const MAX_WINDOW_DAYS: i64 = 30; +const MAX_REPOS: usize = 500; +const PARALLEL_REPOS: usize = 4; +const REPO_TIMEOUT: Duration = Duration::from_secs(30); +const NETWORK_TIMEOUT: Duration = Duration::from_secs(15); +/// Stays below the one-hour retry delay in `db::claim_digest`, so a slow +/// digest is never claimed and sent twice. +const DELIVER_TIMEOUT: Duration = Duration::from_secs(45 * 60); +/// Items listed per section. Counts always cover every alert. +const LIST_ITEMS: usize = 5; + +/// Days an open alert may stay open before the digest calls it overdue. +pub fn deadline_days(severity: &str) -> Option { + match severity { + "critical" => Some(15), + "high" => Some(30), + "medium" => Some(90), + "low" => Some(180), + _ => None, + } +} + +fn severity_rank(severity: &str) -> usize { + ["critical", "high", "medium", "low"] + .iter() + .position(|s| *s == severity) + .unwrap_or(4) +} + +/// Accepts `https://[:443]/` only. IP literals, +/// credentials, other ports, and single-label or local names are rejected. +pub fn validate_webhook_url(url: &str) -> Result<(), &'static str> { + let invalid = "invalid webhook URL"; + if url.len() > 2048 || url.chars().any(|c| c.is_whitespace() || c.is_control()) { + return Err(invalid); + } + let uri: Uri = url.parse().map_err(|_| invalid)?; + if uri.scheme_str() != Some("https") { + return Err("webhook URL must use https"); + } + let authority = uri.authority().ok_or(invalid)?; + if authority.as_str().contains('@') { + return Err("webhook URL must not contain credentials"); + } + if authority.port_u16().is_some_and(|port| port != 443) { + return Err("webhook URL must use port 443"); + } + let host = authority.host().trim_end_matches('.').to_ascii_lowercase(); + let public_name = host.contains('.') + && host.parse::().is_err() + && !host.starts_with('[') + && ![".local", ".localhost", ".internal", ".home.arpa"] + .iter() + .any(|suffix| host.ends_with(suffix)); + if !public_name { + return Err("webhook URL must use a public host name"); + } + Ok(()) +} + +/// The host of a stored webhook URL, for display. The rest is secret. +pub fn webhook_host(url: &str) -> Option { + let uri: Uri = url.parse().ok()?; + Some(uri.host()?.to_ascii_lowercase()) +} + +/// True for globally routable unicast addresses. +pub fn is_public(ip: IpAddr) -> bool { + match ip { + IpAddr::V4(v4) => public_v4(v4), + IpAddr::V6(v6) => { + if let Some(v4) = v6.to_ipv4_mapped() { + return public_v4(v4); + } + let first = v6.segments()[0]; + !(v6.is_loopback() + || v6.is_unspecified() + || v6.is_multicast() + || v6.is_unique_local() + || v6.is_unicast_link_local() + // Site-local, documentation, NAT64, and IPv4-compatible ranges. + || first & 0xffc0 == 0xfec0 + || (first == 0x2001 && v6.segments()[1] == 0x0db8) + || (first == 0x0064 && v6.segments()[1] == 0xff9b) + || v6.segments()[..6].iter().all(|s| *s == 0)) + } + } +} + +fn public_v4(ip: Ipv4Addr) -> bool { + let [a, b, c, _] = ip.octets(); + !(ip.is_private() + || ip.is_loopback() + || ip.is_link_local() + || ip.is_broadcast() + || ip.is_documentation() + || ip.is_unspecified() + || ip.is_multicast() + || a == 0 + || a >= 240 + || (a == 100 && (64..128).contains(&b)) + || (a == 192 && b == 0 && c == 0) + || (a == 198 && (18..20).contains(&b))) +} + +#[derive(Debug, Deserialize)] +pub struct DigestAlert { + pub created_at: DateTime, + pub updated_at: DateTime, + pub fixed_at: Option>, + pub dependency: Dependency, + pub security_advisory: Advisory, + pub security_vulnerability: Vulnerability, +} + +/// Why GitHub did not return the alerts of a repository. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ReadError { + /// 401, 403, or 404: no access, or alerts are off. Retrying does not help. + NoAccess, + /// Timeout, rate limit, or server error. The digest waits and retries. + Temporary, +} + +/// Open and recently fixed alerts of one repository. +pub struct RepoAlerts { + pub repo: String, + pub alerts: Result<(Vec, Vec), ReadError>, +} + +#[derive(Debug, PartialEq, Eq)] +pub struct Entry { + pub repo: String, + pub advisory: String, + pub package: String, + pub severity: String, + pub days_open: i64, +} + +#[derive(Debug)] +pub struct Digest { + pub since: DateTime, + pub until: DateTime, + pub repositories_read: usize, + pub repositories_failed: usize, + /// Alerts created in the window, open or fixed since. + pub new: Vec, + /// Alerts fixed in the window. + pub fixed: Vec, + /// Open alerts older than their severity deadline. + pub overdue: Vec, +} + +fn entry(repo: &str, alert: &DigestAlert, now: DateTime) -> Entry { + let advisory = &alert.security_advisory; + Entry { + repo: repo.to_owned(), + advisory: advisory + .cve_id + .clone() + .unwrap_or_else(|| advisory.ghsa_id.clone()), + package: alert.dependency.package.name.clone(), + severity: alert.security_vulnerability.severity.clone(), + days_open: (now - alert.created_at).num_days(), + } +} + +/// Builds the digest from open alerts and recently fixed alerts. +pub fn summarize(repos: &[RepoAlerts], since: DateTime, now: DateTime) -> Digest { + let mut digest = Digest { + since, + until: now, + repositories_read: 0, + repositories_failed: 0, + new: Vec::new(), + fixed: Vec::new(), + overdue: Vec::new(), + }; + for repo in repos { + let Ok((open, fixed)) = &repo.alerts else { + digest.repositories_failed += 1; + continue; + }; + digest.repositories_read += 1; + for alert in open { + if alert.created_at >= since { + digest.new.push(entry(&repo.repo, alert, now)); + } + let entry = entry(&repo.repo, alert, now); + if deadline_days(&entry.severity).is_some_and(|days| entry.days_open > days) { + digest.overdue.push(entry); + } + } + for alert in fixed { + if alert.fixed_at.is_some_and(|at| at >= since && at <= now) { + if alert.created_at >= since { + digest.new.push(entry(&repo.repo, alert, now)); + } + digest.fixed.push(entry(&repo.repo, alert, now)); + } + } + } + let order = |a: &Entry, b: &Entry| { + severity_rank(&a.severity) + .cmp(&severity_rank(&b.severity)) + .then(b.days_open.cmp(&a.days_open)) + .then(a.repo.cmp(&b.repo)) + .then(a.advisory.cmp(&b.advisory)) + }; + digest.new.sort_by(order); + digest.fixed.sort_by(order); + digest.overdue.sort_by(order); + digest +} + +/// Slack `mrkdwn` needs `&`, `<`, and `>` escaped. This also stops `` +/// mentions from repository or package names. +fn escape(text: &str) -> String { + text.replace('&', "&") + .replace('<', "<") + .replace('>', ">") + .replace('`', "'") +} + +fn by_severity(entries: &[Entry]) -> String { + let mut parts = Vec::new(); + for severity in ["critical", "high", "medium", "low"] { + let count = entries.iter().filter(|e| e.severity == severity).count(); + if count > 0 { + parts.push(format!("{severity} {count}")); + } + } + if parts.is_empty() { + String::new() + } else { + format!(" ({})", parts.join(", ")) + } +} + +pub fn render(account: &str, digest: &Digest) -> String { + let mut text = format!( + "*CroCode weekly security digest for {}*\n{} to {} · {} {} read", + escape(account), + digest.since.format("%Y-%m-%d"), + digest.until.format("%Y-%m-%d"), + digest.repositories_read, + if digest.repositories_read == 1 { + "repository" + } else { + "repositories" + }, + ); + if digest.repositories_failed > 0 { + text.push_str(&format!( + " · {} could not be read", + digest.repositories_failed + )); + } + text.push_str(&format!( + "\n\n• New alerts: {}{}\n• Fixed alerts: {}\n• Overdue open alerts: {}{}\n", + digest.new.len(), + by_severity(&digest.new), + digest.fixed.len(), + digest.overdue.len(), + by_severity(&digest.overdue), + )); + let section = |text: &mut String, title: &str, entries: &[Entry], overdue: bool| { + if entries.is_empty() { + return; + } + text.push_str(&format!("\n*{title}*\n")); + for e in entries.iter().take(LIST_ITEMS) { + text.push_str(&format!( + "• `{}` {} · {} · {}", + escape(&e.advisory), + e.severity, + escape(&e.repo), + escape(&e.package), + )); + if overdue && let Some(days) = deadline_days(&e.severity) { + text.push_str(&format!(" · open {} days (limit {days})", e.days_open)); + } + text.push('\n'); + } + if entries.len() > LIST_ITEMS { + text.push_str(&format!("…and {} more\n", entries.len() - LIST_ITEMS)); + } + }; + section(&mut text, "New alerts", &digest.new, false); + section(&mut text, "Overdue alerts", &digest.overdue, true); + text.push_str( + "\nDeadlines: critical 15 days, high 30, medium 90, low 180. \ + Source: GitHub Dependabot alerts on default branches. This is not a full security scan.", + ); + text +} + +/// Reads alert pages until `more` returns false for a page. +async fn alert_pages( + client: &Octocrab, + repo: &str, + query: &[(&str, &str)], + more: impl Fn(&[T]) -> bool, +) -> Result> { + anyhow::ensure!(valid_repo_name(repo), "invalid repository name"); + let mut page: Page = client + .get(format!("/repos/{repo}/dependabot/alerts"), Some(query)) + .await?; + let mut result = Vec::new(); + loop { + let keep_going = more(&page.items); + result.append(&mut page.items); + if !keep_going { + break; + } + match client.get_page::(&page.next).await? { + Some(next) => page = next, + None => break, + } + } + Ok(result) +} + +async fn repo_alerts( + client: &Octocrab, + repo: &str, + since: DateTime, +) -> Result<(Vec, Vec)> { + let open = alert_pages( + client, + repo, + &[("state", "open"), ("per_page", "100")], + |_| true, + ) + .await?; + // Newest updates first. `fixed_at` is never after `updated_at`, so the + // read stops at the first page that ends before the window. + let fixed = alert_pages( + client, + repo, + &[ + ("state", "fixed"), + ("sort", "updated"), + ("direction", "desc"), + ("per_page", "100"), + ], + |items: &[DigestAlert]| items.last().is_some_and(|a| a.updated_at >= since), + ) + .await?; + Ok((open, fixed)) +} + +/// Reads alerts of up to [`MAX_REPOS`] installation repositories. +pub async fn collect(client: &Octocrab, since: DateTime) -> Result> { + #[derive(Deserialize)] + struct Repository { + full_name: String, + } + #[derive(Deserialize)] + struct Repositories { + repositories: Vec, + } + let mut names = Vec::new(); + for page in 1..=MAX_REPOS.div_ceil(100) as u32 { + let batch: Repositories = client + .get( + "/installation/repositories", + Some(&[("per_page", 100u32), ("page", page)]), + ) + .await + .context("list installation repositories")?; + let done = batch.repositories.len() < 100; + names.extend(batch.repositories.into_iter().map(|r| r.full_name)); + if done { + break; + } + } + names.truncate(MAX_REPOS); + let limit = Arc::new(tokio::sync::Semaphore::new(PARALLEL_REPOS)); + let mut tasks = JoinSet::new(); + for repo in names { + let client = client.clone(); + let limit = limit.clone(); + tasks.spawn(async move { + let _permit = limit.acquire_owned().await; + let alerts = match tokio::time::timeout( + REPO_TIMEOUT, + repo_alerts(&client, &repo, since), + ) + .await + { + Ok(Ok(alerts)) => Ok(alerts), + Ok(Err(err)) => { + warn!(%repo, error = %format!("{err:#}"), "digest: read alerts"); + match crate::vulnerabilities::failure_status(&err) { + CoverageStatus::AccessDenied | CoverageStatus::Unavailable => { + Err(ReadError::NoAccess) + } + _ => Err(ReadError::Temporary), + } + } + Err(_) => { + warn!(%repo, "digest: alert read timed out"); + Err(ReadError::Temporary) + } + }; + RepoAlerts { repo, alerts } + }); + } + let mut repos = Vec::new(); + while let Some(result) = tasks.join_next().await { + repos.push(result?); + } + repos.sort_by(|a, b| a.repo.cmp(&b.repo)); + Ok(repos) +} + +/// Writes one HTTP/1.1 POST and returns the response status code. +async fn post( + mut stream: S, + host: &str, + path: &str, + body: &str, +) -> std::io::Result { + let request = format!( + "POST {path} HTTP/1.1\r\nHost: {host}\r\nUser-Agent: crocode\r\n\ + Content-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", + body.len() + ); + stream.write_all(request.as_bytes()).await?; + stream.flush().await?; + let mut head = Vec::new(); + let mut buf = [0u8; 512]; + while !head.windows(2).any(|w| w == b"\r\n") && head.len() < 4096 { + let n = stream.read(&mut buf).await?; + if n == 0 { + break; + } + head.extend_from_slice(&buf[..n]); + } + let line = String::from_utf8_lossy(&head); + line.strip_prefix("HTTP/1.") + .and_then(|rest| rest.split(' ').nth(1)) + .and_then(|code| code.parse().ok()) + .ok_or_else(|| std::io::Error::other("invalid HTTP response")) +} + +/// Posts `{"text": ...}` to the webhook. Errors never contain the URL. +pub async fn send(url: &str, text: &str) -> Result<(), String> { + validate_webhook_url(url).map_err(str::to_owned)?; + let uri: Uri = url.parse().map_err(|_| "invalid webhook URL".to_owned())?; + let host = uri.host().unwrap_or_default().to_owned(); + let path = uri.path_and_query().map_or("/", |p| p.as_str()).to_owned(); + let addrs: Vec = tokio::time::timeout( + NETWORK_TIMEOUT, + tokio::net::lookup_host((host.as_str(), 443)), + ) + .await + .map_err(|_| "webhook host lookup timed out".to_owned())? + .map_err(|_| "webhook host did not resolve".to_owned())? + .collect(); + // Every address must be public, so DNS cannot mix in an internal target. + if addrs.is_empty() || !addrs.iter().all(|a| is_public(a.ip())) { + return Err("webhook host must resolve only to public addresses".into()); + } + // Try each checked address, so an IPv6 answer on an IPv4-only host still works. + let mut tcp = None; + for addr in &addrs { + if let Ok(Ok(stream)) = + tokio::time::timeout(NETWORK_TIMEOUT, tokio::net::TcpStream::connect(addr)).await + { + tcp = Some(stream); + break; + } + } + let tcp = tcp.ok_or_else(|| "webhook connection failed".to_owned())?; + let roots = rustls::RootCertStore::from_iter(webpki_roots::TLS_SERVER_ROOTS.iter().cloned()); + let config = rustls::ClientConfig::builder_with_provider(Arc::new( + rustls::crypto::ring::default_provider(), + )) + .with_safe_default_protocol_versions() + .map_err(|_| "TLS setup failed".to_owned())? + .with_root_certificates(roots) + .with_no_client_auth(); + let name = rustls::pki_types::ServerName::try_from(host.clone()) + .map_err(|_| "invalid webhook host".to_owned())?; + let body = serde_json::json!({ "text": text }).to_string(); + let status = tokio::time::timeout(NETWORK_TIMEOUT, async { + let tls = tokio_rustls::TlsConnector::from(Arc::new(config)) + .connect(name, tcp) + .await + .map_err(|_| "webhook TLS handshake failed".to_owned())?; + post(tls, &host, &path, &body) + .await + .map_err(|_| "webhook request failed".to_owned()) + }) + .await + .map_err(|_| "webhook timed out".to_owned())??; + if (200..300).contains(&status) { + Ok(()) + } else { + Err(format!("webhook returned HTTP {status}")) + } +} + +/// Returns the start of the alert read on success. +async fn deliver(state: &AppState, job: &db::DigestJob) -> Result, String> { + let now = Utc::now(); + let since = job + .digest_sent_at + .map_or(now - chrono::Duration::days(PERIOD_DAYS), |sent| { + sent.max(now - chrono::Duration::days(MAX_WINDOW_DAYS)) + }); + let client = state.github.alerts_client(job.id).await.map_err(|err| { + warn!(installation = job.id, error = %format!("{err:#}"), "digest: token"); + "GitHub token request failed; check the Dependabot alerts permission".to_owned() + })?; + let repos = collect(&client, since).await.map_err(|err| { + warn!(installation = job.id, error = %format!("{err:#}"), "digest: repositories"); + "GitHub repository list failed".to_owned() + })?; + // Sending now would move the window past changes in these repositories. + let temporary = repos + .iter() + .filter(|r| r.alerts.as_ref().err() == Some(&ReadError::Temporary)) + .count(); + if temporary > 0 { + return Err(format!( + "{temporary} {} could not be read for now", + if temporary == 1 { + "repository" + } else { + "repositories" + } + )); + } + let digest = summarize(&repos, since, now); + // The admin may have turned the digest off or changed the URL during the read. + match db::installation(&state.db, job.id).await { + Ok(Some(i)) + if !i.suspended + && i.digest_webhook_url.as_deref() == Some(job.digest_webhook_url.as_str()) => {} + Ok(_) => return Err("digest settings changed before sending".into()), + Err(_) => return Err("database error before sending".into()), + } + send( + &job.digest_webhook_url, + &render(&job.account_login, &digest), + ) + .await?; + Ok(now) +} + +/// Sends due digests. Runs in every worker; claims keep sends unique. +pub async fn run(state: AppState, shutdown: CancellationToken) { + loop { + loop { + if shutdown.is_cancelled() { + return; + } + let job = match db::claim_digest(&state.db).await { + Ok(Some(job)) => job, + Ok(None) => break, + Err(err) => { + warn!(error = %err, "claim digest"); + break; + } + }; + let result = tokio::select! { + result = tokio::time::timeout(DELIVER_TIMEOUT, deliver(&state, &job)) => { + result.unwrap_or_else(|_| Err("digest timed out".to_owned())) + } + _ = shutdown.cancelled() => return, + }; + match &result { + Ok(_) => info!(installation = job.id, "security digest sent"), + Err(message) => warn!(installation = job.id, %message, "security digest failed"), + } + if let Err(err) = db::finish_digest( + &state.db, + &job, + result.as_ref().copied().map_err(String::as_str), + ) + .await + { + warn!(error = %err, "save digest result"); + } + } + tokio::select! { + _ = tokio::time::sleep(POLL) => {} + _ = shutdown.cancelled() => return, + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use axum::extract::OriginalUri; + use axum::response::IntoResponse; + use axum::{Router, routing::get}; + use serde_json::json; + + #[test] + fn webhook_urls_must_be_public_https() { + for good in [ + "https://hooks.slack.com/services/T0/B0/xyz", + "https://discord.com/api/webhooks/1/abc/slack", + "https://chat.example.org:443/hooks/abc", + ] { + assert_eq!(validate_webhook_url(good), Ok(()), "{good}"); + } + for bad in [ + "http://hooks.slack.com/x", + "https://user:pass@hooks.slack.com/x", + "https://hooks.slack.com:8443/x", + "https://127.0.0.1/x", + "https://[::1]/x", + "https://localhost/x", + "https://printer.local/x", + "https://metadata.internal/x", + "https://hooks.slack.com/a b", + "https://hooks.slack.com/a\r\nX: y", + "not a url", + ] { + assert!(validate_webhook_url(bad).is_err(), "{bad}"); + } + assert_eq!( + webhook_host("https://Hooks.Slack.com/services/secret").as_deref(), + Some("hooks.slack.com") + ); + } + + #[test] + fn only_global_addresses_are_public() { + for ip in [ + "10.0.0.1", + "172.16.0.1", + "192.168.1.1", + "127.0.0.1", + "169.254.169.254", + "100.64.0.1", + "0.0.0.0", + "255.255.255.255", + "198.18.0.1", + "::1", + "fc00::1", + "fe80::1", + "::ffff:10.0.0.1", + "64:ff9b::a00:1", + "2001:db8::1", + ] { + assert!(!is_public(ip.parse().unwrap()), "{ip}"); + } + for ip in [ + "1.1.1.1", + "140.82.112.3", + "2606:4700::1111", + "::ffff:1.1.1.1", + ] { + assert!(is_public(ip.parse().unwrap()), "{ip}"); + } + } + + fn alert( + severity: &str, + cve: &str, + created: DateTime, + fixed: Option>, + ) -> DigestAlert { + serde_json::from_value(alert_json(severity, cve, created, fixed)).unwrap() + } + + fn alert_json( + severity: &str, + cve: &str, + created: DateTime, + fixed: Option>, + ) -> serde_json::Value { + json!({ + "created_at": created, + "updated_at": fixed.unwrap_or(created), + "fixed_at": fixed, + "dependency": {"package": {"ecosystem": "npm", "name": "left"}, "manifest_path": "package.json", "scope": "runtime"}, + "security_advisory": {"ghsa_id": "GHSA-x", "cve_id": cve, "summary": "s"}, + "security_vulnerability": {"severity": severity, "vulnerable_version_range": "< 2", "first_patched_version": null} + }) + } + + #[test] + fn digest_counts_new_fixed_and_overdue_alerts() { + let now = DateTime::from_timestamp(100 * 86_400, 0).unwrap(); + let day = |n: i64| now - chrono::Duration::days(n); + let since = day(7); + let repos = [ + RepoAlerts { + repo: "acme/api".into(), + alerts: Ok(( + vec![ + alert("critical", "CVE-1", day(2), None), + alert("high", "CVE-2", day(31), None), + alert("high", "CVE-3", day(29), None), + alert("low", "CVE-4", day(200), None), + ], + vec![ + alert("medium", "CVE-5", day(3), Some(day(1))), + alert("medium", "CVE-6", day(60), Some(day(6))), + alert("medium", "CVE-7", day(60), Some(day(8))), + ], + )), + }, + RepoAlerts { + repo: "acme/denied".into(), + alerts: Err(ReadError::NoAccess), + }, + ]; + let digest = summarize(&repos, since, now); + assert_eq!( + (digest.repositories_read, digest.repositories_failed), + (1, 1) + ); + let ids = |e: &[Entry]| e.iter().map(|e| e.advisory.clone()).collect::>(); + assert_eq!(ids(&digest.new), ["CVE-1", "CVE-5"]); + assert_eq!(ids(&digest.fixed), ["CVE-6", "CVE-5"]); + assert_eq!(ids(&digest.overdue), ["CVE-2", "CVE-4"]); + + let text = render("acme", &digest); + assert!(text.contains("acme<!channel>")); + assert!(!text.contains("")); + assert!(text.contains("left<pad>")); + assert!(text.contains("New alerts: 2 (critical 1, medium 1)")); + assert!(text.contains("Fixed alerts: 2")); + assert!(text.contains("open 31 days (limit 30)")); + assert!(text.contains("1 could not be read")); + } + + #[tokio::test] + async fn post_reads_the_status_and_sends_one_request() { + let (client, mut server) = tokio::io::duplex(4096); + let reply = tokio::spawn(async move { + let mut buf = vec![0u8; 1024]; + let n = server.read(&mut buf).await.unwrap(); + server + .write_all(b"HTTP/1.1 204 No Content\r\nX: y\r\n\r\n") + .await + .unwrap(); + String::from_utf8_lossy(&buf[..n]).into_owned() + }); + let status = post(client, "hooks.example.com", "/a?b=c", r#"{"text":"hi"}"#) + .await + .unwrap(); + assert_eq!(status, 204); + let request = reply.await.unwrap(); + assert!(request.starts_with("POST /a?b=c HTTP/1.1\r\nHost: hooks.example.com\r\n")); + assert!(request.ends_with("\r\n\r\n{\"text\":\"hi\"}")); + } + + #[tokio::test] + async fn send_refuses_private_targets_before_connecting() { + // `localhost` fails validation; a public-looking name that resolves + // to loopback fails the address check. + assert!(send("https://localhost/x", "t").await.is_err()); + let err = send("https://localtest.me/x", "t").await.unwrap_err(); + assert!( + err == "webhook host must resolve only to public addresses" + || err == "webhook host did not resolve" + || err == "webhook host lookup timed out", + "{err}" + ); + } + + #[tokio::test] + async fn collect_reads_open_and_recent_fixed_alerts() { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let origin = format!("http://{}", listener.local_addr().unwrap()); + let now = Utc::now(); + let old = now - chrono::Duration::days(20); + let next = + format!("<{origin}/repos/acme/api/dependabot/alerts?state=fixed&page=2>; rel=\"next\""); + let app = Router::new().fallback(get(move |OriginalUri(uri): OriginalUri| { + let next = next.clone(); + async move { + let query = uri.query().unwrap_or_default().to_owned(); + match uri.path() { + "/installation/repositories" => axum::Json(json!({"total_count": 3, + "repositories": [{"full_name": "acme/api"}, {"full_name": "acme/gone"}, {"full_name": "acme/limited"}]})) + .into_response(), + "/repos/acme/api/dependabot/alerts" if query.contains("state=open") => { + axum::Json(json!([alert_json("high", "CVE-1", now, None)])).into_response() + } + "/repos/acme/api/dependabot/alerts" if query.contains("page=2") => { + panic!("read past the window") + } + "/repos/acme/api/dependabot/alerts" => { + assert!(query.contains("sort=updated")); + ( + [("link", next)], + axum::Json(json!([alert_json("low", "CVE-2", old, Some(old))])), + ) + .into_response() + } + "/repos/acme/limited/dependabot/alerts" => { + axum::http::StatusCode::TOO_MANY_REQUESTS.into_response() + } + _ => axum::http::StatusCode::NOT_FOUND.into_response(), + } + } + })); + let server = tokio::spawn(async move { axum::serve(listener, app).await.unwrap() }); + let client = Octocrab::builder() + .base_uri(&origin) + .unwrap() + .personal_token("test".to_owned()) + .add_retry_config(octocrab::service::middleware::retry::RetryConfig::None) + .build() + .unwrap(); + let repos = collect(&client, now - chrono::Duration::days(7)) + .await + .unwrap(); + assert_eq!(repos.len(), 3); + let (open, fixed) = repos[0].alerts.as_ref().unwrap(); + assert_eq!((open.len(), fixed.len()), (1, 1)); + assert_eq!(repos[1].alerts.as_ref().err(), Some(&ReadError::NoAccess)); + assert_eq!(repos[2].alerts.as_ref().err(), Some(&ReadError::Temporary)); + server.abort(); + } +} diff --git a/crates/server/src/github.rs b/crates/server/src/github.rs index e65d4f8..5ed2138 100644 --- a/crates/server/src/github.rs +++ b/crates/server/src/github.rs @@ -158,6 +158,39 @@ impl GitHub { if !valid_repo_name(repo) { bail!("invalid repository name {repo:?}"); } + let token = self + .installation_token( + installation_id, + json!({ + "repository_ids": [repo_id], + "permissions": { "contents": "read", "pull_requests": "write" } + }), + ) + .await?; + Ok(RepoClient { + client: self.token_client(&token)?, + token, + repo: repo.to_owned(), + }) + } + + /// Client for every repository of an installation that can only read + /// Dependabot alerts. Used by the weekly security digest. + pub async fn alerts_client(&self, installation_id: i64) -> Result { + let token = self + .installation_token( + installation_id, + json!({ "permissions": { "vulnerability_alerts": "read", "metadata": "read" } }), + ) + .await?; + self.token_client(&token) + } + + async fn installation_token( + &self, + installation_id: i64, + body: serde_json::Value, + ) -> Result { #[derive(Deserialize)] struct Token { token: String, @@ -166,22 +199,18 @@ impl GitHub { .app .post( format!("/app/installations/{installation_id}/access_tokens"), - Some(&json!({ - "repository_ids": [repo_id], - "permissions": { "contents": "read", "pull_requests": "write" } - })), + Some(&body), ) .await .context("create installation token")?; - let client = Octocrab::builder() + Ok(token.token) + } + + fn token_client(&self, token: &str) -> Result { + Ok(Octocrab::builder() .base_uri(self.api_url.as_str())? - .personal_token(token.token.clone()) - .build()?; - Ok(RepoClient { - client, - token: token.token, - repo: repo.to_owned(), - }) + .personal_token(token.to_owned()) + .build()?) } pub fn user_client(&self, token: &str) -> Result { diff --git a/crates/server/src/main.rs b/crates/server/src/main.rs index 4accc4e..c605bf5 100644 --- a/crates/server/src/main.rs +++ b/crates/server/src/main.rs @@ -15,6 +15,7 @@ use tracing::info; mod agents; mod api; mod db; +mod digest; mod github; mod models; mod review; diff --git a/crates/server/src/models.rs b/crates/server/src/models.rs index c3e400b..8ec2464 100644 --- a/crates/server/src/models.rs +++ b/crates/server/src/models.rs @@ -36,14 +36,17 @@ pub struct Model { /// Prints `{"help": , "models": }`. The API /// call needs Anthropic credentials and is skipped for Bedrock and Vertex. -/// An API key wins over a subscription token, like the Claude CLI. +/// An API key wins over a subscription token, like the Claude CLI. A failed +/// `claude --help` exits nonzero, so the check marks the CLI unavailable. const CLAUDE_LIST_SCRIPT: &str = r#" import { execFileSync } from "node:child_process"; const env = process.env; const out = { help: "", models: [] }; try { out.help = execFileSync("claude", ["--help"], { encoding: "utf8", timeout: 60000 }); -} catch {} +} catch { + process.exit(1); +} const key = env.ANTHROPIC_API_KEY; const oauth = env.CLAUDE_CODE_OAUTH_TOKEN; if ((key || oauth) && !env.CLAUDE_CODE_USE_BEDROCK && !env.CLAUDE_CODE_USE_VERTEX) { @@ -233,7 +236,18 @@ pub async fn refresh_loop(state: AppState, shutdown: CancellationToken) { if shutdown.is_cancelled() { return; } - match detect(&state, agent, &shutdown).await { + let result = detect(&state, agent, &shutdown).await; + if shutdown.is_cancelled() { + return; + } + // An empty list means the CLI did not start or answer as expected. + let available = matches!(&result, Ok(models) if !models.is_empty()); + if !available + && let Err(err) = db::mark_agent_unavailable(&state.db, agent.name()).await + { + warn!(error = %err, "save agent check"); + } + match result { Ok(models) if models.is_empty() => { warn!(agent = agent.name(), "agent listed no models") } diff --git a/crates/server/src/review.rs b/crates/server/src/review.rs index 85a81cb..5a1a3d1 100644 --- a/crates/server/src/review.rs +++ b/crates/server/src/review.rs @@ -3,7 +3,7 @@ use std::collections::{HashMap, HashSet}; use std::path::{Path, PathBuf}; use std::process::Stdio; -use std::time::Duration; +use std::time::{Duration, Instant}; use anyhow::{Context, anyhow}; use crocode_core::config::{CONFIG_PATH, RepoConfig}; @@ -28,7 +28,11 @@ pub enum Outcome { Done { summary: String, agent: Agent, + /// The model the agent reported, else the requested model. model: Option, + /// Time in agent processes, including a fallback run. + agent_ms: i64, + usage: agents::Usage, }, Skipped(String), } @@ -57,6 +61,7 @@ fn skip(reason: impl Into) -> Result { pub async fn execute( state: &AppState, job: &db::Job, + worker: &str, cancel: &CancellationToken, ) -> Result { let settings = &state.settings; @@ -207,6 +212,8 @@ pub async fn execute( let mut agent = agent; let mut model = model; let mut fallback_used = false; + let mut agent_ms: i64 = 0; + let mut usage = agents::Usage::default(); let agent_review = loop { if cancel.is_cancelled() { return Err(ReviewError::Cancelled); @@ -238,7 +245,9 @@ pub async fn execute( } else { Vec::new() }; + db::set_review_agent(&state.db, job.id, worker, agent.name(), model.as_deref()).await?; info!(review = %job.id, agent = agent.name(), "agent started"); + let started = Instant::now(); let output = sandbox::run( settings, sandbox::Job { @@ -259,6 +268,7 @@ pub async fn execute( RunError::Cancelled => ReviewError::Cancelled, other => ReviewError::Retry(anyhow!(other)), })?; + agent_ms = agent_ms.saturating_add(started.elapsed().as_millis() as i64); // Agent output can echo credentials if a prompt injection asks for them. // Remove every secret value this worker knows before anything is stored. @@ -272,6 +282,7 @@ pub async fn execute( } secrets.extend(codex_secrets); let stdout = redact(&output.stdout, &secrets); + usage.add(agents::parse_usage(agent, &stdout)); let result_file = tokio::fs::read_to_string(output_dir.join(RESULT_FILE)) .await .ok() @@ -326,10 +337,13 @@ pub async fn execute( max_inline: config.max_comments, }, ); - let seen = db::published_fingerprints(&state.db, job.repo_id, number).await?; + let seen = db::published_fingerprints(&state.db, job.repo_id, number, job.id).await?; + // Findings that an earlier attempt of this review posted keep their rows. + let own = db::review_published_fingerprints(&state.db, job.id).await?; let (new, repeated): (Vec, Vec) = validated .findings .into_iter() + .filter(|f| !own.contains(&f.fingerprint)) .partition(|f| !seen.contains(&f.fingerprint)); info!( review = %job.id, @@ -339,20 +353,12 @@ pub async fn execute( "agent finished" ); - // A retry after a crash must not post the same review twice. - let mut posted = job.github_review_id.is_some(); - if !posted && !new.is_empty() { - let review_id = - publish(&gh, number, &job.head_sha, agent, &validated.summary, &new).await?; - db::set_github_review_id(&state.db, job.id, review_id).await?; - posted = true; - } - + // Store first, unpublished. The posted findings are marked in the same + // transaction that saves the GitHub review ID, so the record never guesses. let rows: Vec> = new .iter() - .map(|f| (f, posted)) - .chain(repeated.iter().map(|f| (f, false))) - .map(|(f, published)| db::NewFinding { + .chain(repeated.iter()) + .map(|f| db::NewFinding { fingerprint: &f.fingerprint, path: &f.path, start_line: f.start_line, @@ -362,7 +368,7 @@ pub async fn execute( title: &f.title, body: &f.body, suggestion: f.suggestion.as_deref(), - published, + published: false, }) .collect(); let finding_paths: HashSet<&str> = rows.iter().map(|f| f.path).collect(); @@ -374,6 +380,19 @@ pub async fn execute( .collect(); db::insert_findings(&state.db, job.id, job.repo_id, number, &rows, &patches).await?; + // A review is posted at most once, even across retries. + if job.github_review_id.is_none() && !new.is_empty() { + // A dashboard cancel or a newer push may have taken the review away + // after the last heartbeat. + if !db::heartbeat(&state.db, job.id, worker).await? { + return Err(ReviewError::Cancelled); + } + let review_id = + publish(&gh, number, &job.head_sha, agent, &validated.summary, &new).await?; + let fingerprints: Vec<&str> = new.iter().map(|f| f.fingerprint.as_str()).collect(); + db::mark_published(&state.db, job.id, review_id, &fingerprints).await?; + } + let summary = if new.is_empty() { format!("{}\n\nNo new findings.", validated.summary) } else { @@ -382,7 +401,9 @@ pub async fn execute( Ok(Outcome::Done { summary, agent, - model, + model: usage.model.clone().or(model), + agent_ms, + usage, }) } diff --git a/crates/server/src/tests.rs b/crates/server/src/tests.rs index 6475c23..388c951 100644 --- a/crates/server/src/tests.rs +++ b/crates/server/src/tests.rs @@ -256,7 +256,9 @@ async fn heartbeat_stops_after_cancel(pool: PgPool) { db::claim_review(&pool, "worker", 1).await.unwrap().unwrap(); assert!(db::heartbeat(&pool, id, "worker").await.unwrap()); assert!(!db::heartbeat(&pool, id, "other").await.unwrap()); - db::cancel_pr_reviews(&pool, 10, 1, None).await.unwrap(); + db::cancel_pr_reviews(&pool, 10, 1, None, "superseded") + .await + .unwrap(); assert!(!db::heartbeat(&pool, id, "worker").await.unwrap()); db::release_review(&pool, id, "worker").await.unwrap(); assert_eq!( @@ -399,7 +401,7 @@ fn finding(fingerprint: &str, published: bool) -> db::NewFinding<'_> { #[sqlx::test(migrations = "../../migrations")] #[ignore = "needs DATABASE_URL"] -async fn findings_replace_on_retry_and_dedupe_only_published_in_same_pr(pool: PgPool) { +async fn findings_retry_keeps_published_rows_and_dedupes_in_same_pr(pool: PgPool) { installation(&pool, 1).await; let id = enqueue(&pool, 1, 1).await; let other_pr = enqueue(&pool, 1, 2).await; @@ -411,48 +413,85 @@ async fn findings_replace_on_retry_and_dedupe_only_published_in_same_pr(pool: Pg id, 10, 1, - &[finding("old", true), finding("unpublished", false)], + &[finding("posted", true), finding("unpublished", false)], &[], ) .await .unwrap(); assert_eq!(db::review_contents(&pool, id).await.unwrap().0.len(), 2); assert_eq!( - db::published_fingerprints(&pool, 10, 1).await.unwrap(), - ["old".into()].into() + db::published_fingerprints(&pool, 10, 1, Uuid::nil()) + .await + .unwrap(), + ["posted".into()].into() ); assert!( - db::published_fingerprints(&pool, 20, 1) + db::published_fingerprints(&pool, 20, 1, Uuid::nil()) + .await + .unwrap() + .is_empty() + ); + // A retried review does not count its own posts as seen elsewhere. + assert!( + db::published_fingerprints(&pool, 10, 1, id) .await .unwrap() .is_empty() ); - db::insert_findings(&pool, id, 10, 1, &[finding("new", true)], &[]) - .await - .unwrap(); - let rows = db::review_contents(&pool, id).await.unwrap().0; - assert_eq!(rows.len(), 1); assert_eq!( - ( - rows[0].path.as_str(), - rows[0].start_line, - rows[0].line, - rows[0].published - ), - ("src/login.rs", Some(3), 4, true) + db::review_published_fingerprints(&pool, id).await.unwrap(), + ["posted".into()].into() ); + + // The retry replaces unpublished rows and keeps the record of what + // GitHub received. + db::insert_findings(&pool, id, 10, 1, &[finding("retry", false)], &[]) + .await + .unwrap(); + let mut rows: Vec<(String, bool)> = db::review_contents(&pool, id) + .await + .unwrap() + .0 + .into_iter() + .map(|f| (f.title, f.published)) + .collect(); + rows.sort(); + assert_eq!(rows.len(), 2); + assert!(rows.iter().any(|(_, published)| *published)); + assert!(rows.iter().any(|(_, published)| !*published)); assert_eq!( - db::published_fingerprints(&pool, 10, 1).await.unwrap(), - ["new".into()].into() + db::published_fingerprints(&pool, 10, 1, Uuid::nil()) + .await + .unwrap(), + ["posted".into()].into() ); assert_eq!( db::review_contents(&pool, other_pr).await.unwrap().0.len(), 1 ); - db::insert_findings(&pool, id, 10, 1, &[], &[]) - .await - .unwrap(); - assert!(db::review_contents(&pool, id).await.unwrap().0.is_empty()); + // Posting marks exactly the posted findings and saves the review ID. + db::insert_findings( + &pool, + id, + 10, + 1, + &[finding("sent", false), finding("kept", false)], + &[], + ) + .await + .unwrap(); + db::mark_published(&pool, id, 99, &["sent"]).await.unwrap(); + assert_eq!( + db::review_published_fingerprints(&pool, id).await.unwrap(), + ["posted".into(), "sent".into()].into() + ); + let github_review_id: Option = + sqlx::query_scalar("SELECT github_review_id FROM reviews WHERE id = $1") + .bind(id) + .fetch_one(&pool) + .await + .unwrap(); + assert_eq!(github_review_id, Some(99)); } #[sqlx::test(migrations = "../../migrations")] @@ -775,6 +814,8 @@ async fn agent_models_replace_and_show_only_enabled_agents(pool: PgPool) { db::save_agent_models(&pool, "retired", &[model("x")]) .await .unwrap(); + // A failed check keeps the models of the last success. + db::mark_agent_unavailable(&pool, "codex").await.unwrap(); let mut state = state(pool); let mut settings = Settings::for_test(); settings.agents = vec![crate::agents::Agent::Codex, crate::agents::Agent::Claude]; @@ -808,12 +849,15 @@ async fn agent_models_replace_and_show_only_enabled_agents(pool: PgPool) { assert_eq!(response.status(), StatusCode::OK); let body: Value = serde_json::from_slice(&to_bytes(response.into_body(), 4096).await.unwrap()).unwrap(); + let checked_at = body["health"]["codex"]["checked_at"].clone(); + assert!(checked_at.is_string()); assert_eq!( body, json!({ "agents": ["codex", "claude"], "default": "codex", "models": {"codex": [{"id": "gpt-6", "name": "gpt-6"}], "claude": []}, + "health": {"codex": {"available": false, "checked_at": checked_at}, "claude": null}, }) ); } @@ -862,3 +906,330 @@ async fn vulnerability_access_and_page_validation_need_no_external_io() { assert_eq!(response.status(), status, "{path}"); } } + +fn with_access(state: &AppState, token: &str, installations: &[(i64, bool)]) { + let key: [u8; 32] = ::digest(token.as_bytes()).into(); + state.auth.put( + key, + crate::api::Access { + user: crate::github::User { + id: 1, + login: "octocat".into(), + }, + installations: installations.iter().copied().collect(), + }, + ); +} + +async fn call(state: &AppState, method: &str, path: &str, token: &str) -> (StatusCode, Value) { + let response = router(state.clone()) + .oneshot( + Request::builder() + .method(method) + .uri(path) + .header("authorization", format!("Bearer {token}")) + .header("content-type", "application/json") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + let status = response.status(); + let body = to_bytes(response.into_body(), 65536).await.unwrap(); + (status, serde_json::from_slice(&body).unwrap_or(Value::Null)) +} + +async fn enqueue_sha(pool: &PgPool, sha: &str) -> Uuid { + db::enqueue_review( + pool, + &db::NewReview { + installation_id: 1, + repo_id: 10, + repo_full_name: "acme/api", + pr_number: 1, + pr_title: "Fix login", + head_sha: sha, + base_sha: "base", + }, + ) + .await + .unwrap() + .unwrap() +} + +#[sqlx::test(migrations = "../../migrations")] +#[ignore = "needs DATABASE_URL"] +async fn review_controls_cancel_and_retry_only_valid_reviews(pool: PgPool) { + installation(&pool, 1).await; + let first = enqueue_sha(&pool, "head1").await; + assert!(!db::requeue_review(&pool, first).await.unwrap()); + assert!(db::cancel_review(&pool, first).await.unwrap()); + assert!(!db::cancel_review(&pool, first).await.unwrap()); + let row = db::review(&pool, first).await.unwrap().unwrap().0; + assert_eq!( + (row.status.as_str(), row.error.as_deref()), + ("cancelled", Some("cancelled from the dashboard")) + ); + + assert!(db::requeue_review(&pool, first).await.unwrap()); + let row = db::review(&pool, first).await.unwrap().unwrap().0; + assert_eq!( + (row.status.as_str(), row.attempts, row.error), + ("queued", 0, None) + ); + + // Cancelling a running review takes the lease away from its worker. + db::claim_review(&pool, "worker", 1).await.unwrap().unwrap(); + db::set_review_agent(&pool, first, "worker", "claude", Some("opus")) + .await + .unwrap(); + db::mark_published(&pool, first, 77, &[]).await.unwrap(); + assert!(db::cancel_review(&pool, first).await.unwrap()); + assert!(!db::heartbeat(&pool, first, "worker").await.unwrap()); + // The stopping worker still holds the lease, so a retry must wait. + assert!(!db::requeue_review(&pool, first).await.unwrap()); + db::finish_review( + &pool, + first, + "worker", + &db::Finish { + status: "succeeded", + ..Default::default() + }, + ) + .await + .unwrap(); + assert_eq!( + db::review(&pool, first).await.unwrap().unwrap().0.status, + "cancelled" + ); + db::release_lock(&pool, first, "other").await.unwrap(); + assert!(!db::requeue_review(&pool, first).await.unwrap()); + db::release_lock(&pool, first, "worker").await.unwrap(); + assert!(db::requeue_review(&pool, first).await.unwrap()); + let (github_review_id, agent): (Option, Option) = + sqlx::query_as("SELECT github_review_id, agent FROM reviews WHERE id = $1") + .bind(first) + .fetch_one(&pool) + .await + .unwrap(); + assert_eq!((github_review_id, agent), (Some(77), None)); + + // A newer commit makes the old review final and blocks its retry. + let second = enqueue_sha(&pool, "head2").await; + assert_eq!( + db::review(&pool, first).await.unwrap().unwrap().0.status, + "cancelled" + ); + assert!(!db::requeue_review(&pool, first).await.unwrap()); + sqlx::query("UPDATE reviews SET status = 'failed', error = 'worker lost' WHERE id = $1") + .bind(second) + .execute(&pool) + .await + .unwrap(); + assert!(!db::requeue_review(&pool, first).await.unwrap()); + + let state = state(pool.clone()); + with_access(&state, "admin", &[(1, true)]); + with_access(&state, "member", &[(1, false)]); + with_access(&state, "stranger", &[(2, true)]); + let retry = format!("/api/v1/reviews/{second}/retry"); + let cancel = format!("/api/v1/reviews/{second}/cancel"); + assert_eq!( + call(&state, "POST", &retry, "stranger").await.0, + StatusCode::NOT_FOUND + ); + assert_eq!( + call(&state, "POST", &retry, "member").await.0, + StatusCode::FORBIDDEN + ); + assert_eq!( + call(&state, "POST", &cancel, "admin").await.0, + StatusCode::CONFLICT + ); + let (status, body) = call(&state, "POST", &retry, "admin").await; + assert_eq!( + (status, body["status"].as_str()), + (StatusCode::OK, Some("queued")) + ); + assert_eq!( + call(&state, "POST", &retry, "admin").await.0, + StatusCode::CONFLICT + ); + let (status, body) = call(&state, "POST", &cancel, "admin").await; + assert_eq!( + (status, body["status"].as_str()), + (StatusCode::OK, Some("cancelled")) + ); +} + +#[sqlx::test(migrations = "../../migrations")] +#[ignore = "needs DATABASE_URL"] +async fn usage_is_stored_and_grouped_by_agent(pool: PgPool) { + installation(&pool, 1).await; + let done = enqueue(&pool, 1, 1).await; + let failed = enqueue(&pool, 1, 2).await; + for (id, agent) in [(done, "claude"), (failed, "codex")] { + let job = db::claim_review(&pool, "worker", 5).await.unwrap().unwrap(); + assert_eq!(job.id, id); + db::set_review_agent(&pool, id, "worker", agent, None) + .await + .unwrap(); + } + db::finish_review( + &pool, + done, + "worker", + &db::Finish { + status: "succeeded", + agent: Some("claude"), + model: Some("claude-opus-5-5"), + agent_ms: Some(4_000), + input_tokens: Some(1_000), + cached_input_tokens: Some(600), + output_tokens: Some(50), + ..Default::default() + }, + ) + .await + .unwrap(); + db::finish_review( + &pool, + failed, + "worker", + &db::Finish { + status: "failed", + error: Some("agent timed out after 900s"), + ..Default::default() + }, + ) + .await + .unwrap(); + let row = db::review(&pool, done).await.unwrap().unwrap().0; + assert_eq!( + ( + row.agent_duration_ms, + row.input_tokens, + row.cached_input_tokens, + row.output_tokens + ), + (Some(4_000), Some(1_000), Some(600), Some(50)) + ); + let stats = db::stats(&pool, 1).await.unwrap(); + let agents: Vec<(&str, i64, i64, i64, i64, i64)> = stats + .agents + .iter() + .map(|a| { + ( + a.name.as_str(), + a.reviews, + a.succeeded, + a.failed, + a.usage_reviews, + a.input_tokens, + ) + }) + .collect(); + assert_eq!( + agents, + [("claude", 1, 1, 0, 1, 1_000), ("codex", 1, 0, 1, 0, 0)] + ); + assert_eq!(stats.agents[0].median_agent_secs, Some(4.0)); + assert_eq!(stats.agents[0].models, ["claude-opus-5-5"]); + assert!(stats.agents[1].models.is_empty()); +} + +#[sqlx::test(migrations = "../../migrations")] +#[ignore = "needs DATABASE_URL"] +async fn digest_opt_in_claims_once_and_hides_the_url(pool: PgPool) { + installation(&pool, 1).await; + installation(&pool, 2).await; + assert!(db::claim_digest(&pool).await.unwrap().is_none()); + let url = "https://hooks.example.com/services/secret-path"; + db::update_settings(&pool, 1, true, None, None, Some(Some(url))) + .await + .unwrap(); + let job = db::claim_digest(&pool).await.unwrap().unwrap(); + assert_eq!((job.id, job.digest_webhook_url.as_str()), (1, url)); + assert!(db::claim_digest(&pool).await.unwrap().is_none()); + db::finish_digest(&pool, &job, Err("webhook returned HTTP 404")) + .await + .unwrap(); + let i = db::installation(&pool, 1).await.unwrap().unwrap(); + assert_eq!(i.digest_error.as_deref(), Some("webhook returned HTTP 404")); + assert!(i.digest_sent_at.is_none()); + + // A failed send waits an hour before the next attempt. + assert!(db::claim_digest(&pool).await.unwrap().is_none()); + sqlx::query("UPDATE installations SET digest_attempted_at = now() - interval '2 hours'") + .execute(&pool) + .await + .unwrap(); + let job = db::claim_digest(&pool).await.unwrap().unwrap(); + let covered = Utc::now() - chrono::Duration::minutes(5); + db::finish_digest(&pool, &job, Ok(covered)).await.unwrap(); + let i = db::installation(&pool, 1).await.unwrap().unwrap(); + assert!(i.digest_error.is_none()); + assert_eq!( + i.digest_sent_at.map(|t| t.timestamp_micros()), + Some(covered.timestamp_micros()), + "The next window starts where this read started" + ); + sqlx::query("UPDATE installations SET digest_attempted_at = now() - interval '2 hours'") + .execute(&pool) + .await + .unwrap(); + assert!(db::claim_digest(&pool).await.unwrap().is_none()); + + // A result for an old URL is dropped after the admin changes it. + let other = "https://hooks.example.com/services/other"; + db::update_settings(&pool, 1, true, None, None, Some(Some(other))) + .await + .unwrap(); + db::finish_digest(&pool, &job, Err("old")).await.unwrap(); + let i = db::installation(&pool, 1).await.unwrap().unwrap(); + assert!(i.digest_error.is_none() && i.digest_attempted_at.is_none()); + // A new URL gets its first digest at once. + assert!(i.digest_sent_at.is_none()); + assert_eq!(db::claim_digest(&pool).await.unwrap().unwrap().id, 1); + + let state = state(pool.clone()); + with_access(&state, "admin", &[(1, true)]); + for bad in [ + "http://hooks.example.com/x", + "https://10.0.0.1/x", + "https://localhost/x", + ] { + let response = router(state.clone()) + .oneshot( + Request::patch("/api/v1/installations/1/settings") + .header("authorization", "Bearer admin") + .header("content-type", "application/json") + .body(Body::from(json!({ "digest_webhook_url": bad }).to_string())) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(response.status(), StatusCode::UNPROCESSABLE_ENTITY, "{bad}"); + } + let (status, body) = call(&state, "GET", "/api/v1/installations/1", "admin").await; + assert_eq!(status, StatusCode::OK); + assert_eq!(body["digest"]["enabled"], true); + assert_eq!(body["digest"]["webhook_host"], "hooks.example.com"); + assert!(!body.to_string().contains("services/")); + + db::update_settings(&pool, 1, true, None, None, Some(None)) + .await + .unwrap(); + let i = db::installation(&pool, 1).await.unwrap().unwrap(); + assert!(i.digest_webhook_url.is_none() && i.digest_sent_at.is_none()); + // `None` leaves the digest unchanged. + db::update_settings(&pool, 2, true, None, None, Some(Some(url))) + .await + .unwrap(); + db::update_settings(&pool, 2, false, None, None, None) + .await + .unwrap(); + let i = db::installation(&pool, 2).await.unwrap().unwrap(); + assert_eq!(i.digest_webhook_url.as_deref(), Some(url)); +} diff --git a/crates/server/src/vulnerabilities.rs b/crates/server/src/vulnerabilities.rs index 428306a..2b76e21 100644 --- a/crates/server/src/vulnerabilities.rs +++ b/crates/server/src/vulnerabilities.rs @@ -120,7 +120,7 @@ pub struct RepositoryAlerts { pub last_success_at: Option>, } -fn failure_status(error: &anyhow::Error) -> CoverageStatus { +pub(crate) fn failure_status(error: &anyhow::Error) -> CoverageStatus { let Some(octocrab::Error::GitHub { source, .. }) = error.downcast_ref::() else { return CoverageStatus::Failed; diff --git a/crates/server/src/webhook.rs b/crates/server/src/webhook.rs index 63b87e4..d3a0be9 100644 --- a/crates/server/src/webhook.rs +++ b/crates/server/src/webhook.rs @@ -197,7 +197,12 @@ async fn on_pull_request(state: &AppState, body: &[u8]) -> anyhow::Result<&'stat match e.action.as_str() { "opened" | "reopened" | "synchronize" | "ready_for_review" => {} "closed" | "converted_to_draft" => { - db::cancel_pr_reviews(&state.db, e.repository.id, pr.number, None).await?; + let reason = if e.action == "closed" { + "pull request closed" + } else { + "pull request converted to draft" + }; + db::cancel_pr_reviews(&state.db, e.repository.id, pr.number, None, reason).await?; return Ok("reviews cancelled"); } _ => return Ok("ignored action"), diff --git a/crates/server/src/worker.rs b/crates/server/src/worker.rs index 6a1ee86..238329e 100644 --- a/crates/server/src/worker.rs +++ b/crates/server/src/worker.rs @@ -35,6 +35,7 @@ pub async fn run(state: AppState, shutdown: CancellationToken) { } tasks.spawn(reaper(state.clone(), shutdown.clone())); tasks.spawn(crate::models::refresh_loop(state.clone(), shutdown.clone())); + tasks.spawn(crate::digest::run(state.clone(), shutdown.clone())); while tasks.join_next().await.is_some() {} info!("worker stopped"); } @@ -103,7 +104,7 @@ async fn process(state: &AppState, worker: &str, job: db::Job, shutdown: &Cancel }) }; - let result = review::execute(state, &job, &cancel).await; + let result = review::execute(state, &job, worker, &cancel).await; heartbeat.abort(); let outcome = match result { @@ -111,6 +112,8 @@ async fn process(state: &AppState, worker: &str, job: db::Job, shutdown: &Cancel summary, agent, model, + agent_ms, + usage, }) => { info!("review succeeded"); db::finish_review( @@ -120,9 +123,13 @@ async fn process(state: &AppState, worker: &str, job: db::Job, shutdown: &Cancel &db::Finish { status: "succeeded", summary: Some(&summary), - error: None, agent: Some(agent.name()), model: model.as_deref(), + agent_ms: Some(agent_ms), + input_tokens: usage.input_tokens, + cached_input_tokens: usage.cached_input_tokens, + output_tokens: usage.output_tokens, + ..Default::default() }, ) .await @@ -136,9 +143,7 @@ async fn process(state: &AppState, worker: &str, job: db::Job, shutdown: &Cancel &db::Finish { status: "skipped", summary: Some(&reason), - error: None, - agent: None, - model: None, + ..Default::default() }, ) .await @@ -156,10 +161,7 @@ async fn process(state: &AppState, worker: &str, job: db::Job, shutdown: &Cancel worker, &db::Finish { status: "cancelled", - summary: None, - error: None, - agent: None, - model: None, + ..Default::default() }, ) .await @@ -174,8 +176,7 @@ async fn process(state: &AppState, worker: &str, job: db::Job, shutdown: &Cancel status: "failed", summary: None, error: Some(&message), - agent: None, - model: None, + ..Default::default() }, ) .await @@ -196,8 +197,7 @@ async fn process(state: &AppState, worker: &str, job: db::Job, shutdown: &Cancel status: "failed", summary: None, error: Some(&message), - agent: None, - model: None, + ..Default::default() }, ) .await @@ -207,6 +207,11 @@ async fn process(state: &AppState, worker: &str, job: db::Job, shutdown: &Cancel if let Err(err) = outcome { error!(error = %err, "save review state"); } + // The agent has stopped. A review that another writer finished, such as a + // dashboard cancel, can now be retried. + if let Err(err) = db::release_lock(&state.db, job.id, worker).await { + warn!(error = %err, "release review lease"); + } } /// 30 s, 60 s, 120 s, … up to 10 minutes. diff --git a/docs/api.md b/docs/api.md index b43bcc1..2ea31e1 100644 --- a/docs/api.md +++ b/docs/api.md @@ -18,7 +18,7 @@ Errors use this shape: { "error": "message" } ``` -Status codes: `401` for a missing or bad token, `403` for no access, `404` for unknown IDs, `422` for bad input. +Status codes: `401` for a missing or bad token, `403` for no access, `404` for unknown IDs, `409` for a review in the wrong state, `422` for bad input. ## Endpoints @@ -41,12 +41,22 @@ Agents that this server can run, and the models that each agent CLI reports. "agents": ["claude", "codex"], "default": "claude", "models": { - "claude": [{ "id": "opus", "name": "opus" }], + "claude": [{ "id": "opus", "name": "Latest: Claude Opus 5.5" }], "codex": [{ "id": "gpt-6", "name": "GPT-6" }] + }, + "health": { + "claude": { "available": true, "checked_at": "2026-11-02T10:00:00Z" }, + "codex": null } } ``` +`health` shows the result of the latest model list run for each agent. +`available` is false when the run failed or listed no models. +A failed run keeps the models of the last success. +`health` is `null` until the first run. The check shows that the CLI starts in +the sandbox. It does not prove that review credentials work. + The worker detects models at start and every 6 hours. It runs each list command in the sandbox: @@ -76,6 +86,13 @@ Installations that the user can access and that exist in the database. "suspended": false, "created_at": "2026-11-01T10:00:00Z", "settings": { "enabled": true, "agent": "claude", "model": null }, + "digest": { + "enabled": true, + "webhook_host": "hooks.slack.com", + "sent_at": "2026-11-01T10:00:00Z", + "attempted_at": "2026-11-01T10:00:00Z", + "error": null + }, "can_admin": true } ] @@ -84,6 +101,8 @@ Installations that the user can access and that exist in the database. `account_id` is the GitHub user or organization ID. `can_admin` is true when the user owns the account or is an organization admin. `settings.agent` and `settings.model` are `null` when the installation uses the server default. +`digest` describes the weekly security digest. The API never returns the webhook URL. It returns the host only. +`digest.error` holds the reason for the last failed send. It is `null` after a successful send. ### `GET /api/v1/installations/{id}` @@ -94,9 +113,20 @@ One installation, same shape as above. Needs `can_admin`. All fields are optional. Set `model` to `null` to use the agent default. ```json -{ "enabled": true, "agent": "codex", "model": "gpt-5.1-codex" } +{ + "enabled": true, + "agent": "codex", + "model": "gpt-5.1-codex", + "digest_webhook_url": "https://hooks.slack.com/services/T000/B000/XXXX" +} ``` +Set `digest_webhook_url` to turn on the weekly security digest. Set it to `null` to turn it off. +Leave it out to keep the current value. The URL must use `https`, port 443, and a public host name. +The API rejects IP addresses, credentials in the URL, and local names with `422`. +A new URL clears the last result. The first digest to it goes out within about 10 minutes. +See [Weekly security digest](configuration.md#weekly-security-digest). + Returns the updated installation. ### `GET /api/v1/installations/{id}/reviews?limit=50&before=` @@ -121,13 +151,32 @@ Set both or neither. Otherwise the API returns `422`. "finding_count": 3, "error": null, "created_at": "2026-11-01T10:00:00Z", - "finished_at": "2026-11-01T10:03:00Z" + "finished_at": "2026-11-01T10:03:00Z", + "agent_duration_ms": 95000, + "input_tokens": 24388, + "cached_input_tokens": 12288, + "output_tokens": 512 } ] ``` `status` is one of `queued`, `running`, `succeeded`, `failed`, `cancelled`, `skipped`. -`agent` is `null` until a worker picks an agent for the review. +`agent` is `null` until a worker starts an agent for the review. The worker records the agent before the run. +`model` is the model that the agent reported, or the requested model. `null` means the agent default. + +`agent_duration_ms` is the time in agent processes, including a fallback run. It is set for succeeded reviews. +The token fields are the counts that the agent CLI reported, summed over fallback runs. +Input counts include cached input. Each field is `null` when the CLI reported no value. + +| Agent | Source | +| --- | --- | +| Claude Code | `modelUsage` in the JSON result. Input is input plus cache reads plus cache writes. | +| Codex | `usage` of each `turn.completed` JSONL event. The worker runs `codex exec --json`. | +| OpenCode | `part.tokens` of each `step_finish` event. Output includes reasoning tokens. | + +`error` holds the failure reason of a failed review. For a cancelled review it holds one of +`superseded`, `pull request closed`, `pull request converted to draft`, `installation removed`, +or `cancelled from the dashboard`. ### `GET /api/v1/installations/{id}/stats` @@ -147,13 +196,27 @@ Totals for the last 30 days, the review retention period. "published": 7, "severity": [{ "name": "high", "count": 4 }], "categories": [{ "name": "bug", "count": 5 }], - "daily": [{ "date": "2026-10-04", "reviews": 0, "failed": 0 }] + "daily": [{ "date": "2026-10-04", "reviews": 0, "failed": 0 }], + "agents": [{ + "name": "claude", + "reviews": 8, + "succeeded": 7, + "failed": 1, + "median_agent_secs": 95.0, + "usage_reviews": 7, + "input_tokens": 170716, + "cached_input_tokens": 86016, + "output_tokens": 3584, + "models": ["claude-opus-5-5"] + }] } ``` `median_duration_secs` is the time from queue to result for succeeded reviews. It is `null` when no review succeeded. `categories` holds the five most common. `daily` has one entry for each UTC day, oldest first. +`agents` has one entry for each agent that started a review, most used first. +`usage_reviews` counts the reviews that reported tokens. The token sums cover only those reviews. ### `GET /api/v1/installations/{id}/pulls` @@ -246,6 +309,26 @@ Live pull request details from GitHub, read with the user token. `state` is `open`, `closed`, or `merged`. The API returns `404` when GitHub does not show the pull request to the user, and `502` for other GitHub failures. +### `POST /api/v1/reviews/{id}/retry` + +Needs `can_admin`. Queues a failed or cancelled review again with fresh attempts. No body. +Returns the review with status `queued`. + +The review must be the newest review of its pull request, and no other review of it may be queued or running. +Otherwise the API returns `409`. This rule stops an old commit from replacing a newer one. +A cancelled running review also returns `409` until its worker stops the agent, within about 20 seconds. +This keeps two runs of one review apart. +A retry clears the error, agent, model, timing, and token fields. It keeps the GitHub review ID, +so a review that was already posted is not posted twice. The queue time restarts. + +### `POST /api/v1/reviews/{id}/cancel` + +Needs `can_admin`. Cancels a queued or running review. No body. +Returns the review with status `cancelled` and error `cancelled from the dashboard`. +Other states return `409`. A running agent stops at the next worker heartbeat, within about 20 seconds. + +Both endpoints return `404` for a review that the user cannot see and `403` for a user without `can_admin`. + ### `GET /api/v1/installations/{id}/vulnerabilities` Live open Dependabot alerts for repositories in this installation that the signed-in user can access. diff --git a/docs/configuration.md b/docs/configuration.md index 4d8c4bc..869408d 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -153,6 +153,45 @@ Sandbox networking must permit HTTPS to `api.osv.dev`. `CROCODE_SANDBOX_NETWORK=none` prevents OSV lookups. The tools do not give agents shell, write, or arbitrary URL access. +## Weekly security digest + +The weekly security digest is off by default. An installation admin turns it on in +**Settings** with an HTTPS chat webhook URL. The digest posts a Slack-compatible +JSON body, `{"text": "..."}`. Slack and Mattermost incoming webhooks accept it. +For Discord, add `/slack` to the end of the webhook URL. + +Every 7 days, the worker reads the Dependabot alerts of all installation repositories. +It uses a token that can only read Dependabot alerts and repository metadata. +The digest covers the time since the start of the last successful read, at most 30 days. +The next window starts where the last read started, so no alert falls between two digests: + +- **New alerts**: alerts created in that time that are open or fixed. +- **Fixed alerts**: alerts fixed in that time. +- **Overdue open alerts**: open alerts older than the deadline for their severity. + +| Severity | Deadline | +| --- | --- | +| `critical` | 15 days | +| `high` | 30 days | +| `medium` | 90 days | +| `low` | 180 days | + +Each section lists at most 5 alerts. The counts include every alert. +The digest names repositories, packages, and CVE or GHSA IDs. It contains no advisory text. +It reads at most 500 repositories. A repository that answers 401, 403, or 404 counts as +"could not be read", and the digest still goes out. These answers mean no access or disabled alerts. +A timeout, rate limit, or server error stops the send. The worker tries again in an hour with the +same window, so no alert change is lost. +Dismissed alerts are not counted. This is GitHub's alert inventory, not a full security scan. + +The worker checks for due digests every 10 minutes. After a failed send it tries again every hour. +The settings page shows the last send and the last error. +The GitHub App needs the read-only **Dependabot alerts** permission. See +[Enable the vulnerability panel](github-app.md#enable-the-vulnerability-panel-on-an-existing-app). + +The worker needs outbound HTTPS to the webhook host. It connects only to public addresses. +See [Network and data exposure](security.md#network-and-data-exposure). + ## Compose environment | Variable | Example or default | Meaning | diff --git a/docs/github-app.md b/docs/github-app.md index 774c3c8..d05db3a 100644 --- a/docs/github-app.md +++ b/docs/github-app.md @@ -110,3 +110,5 @@ An invalid signature returns `401` before the handler records a delivery. The dashboard uses the signed-in user's access. The user must be allowed to read security alerts. Repositories without that access show a warning. Review sandbox tokens do not receive this permission. +The opt-in weekly security digest also needs this permission. The worker reads alerts with an +installation token limited to Dependabot alerts and metadata. diff --git a/docs/security.md b/docs/security.md index 8501e3c..028a460 100644 --- a/docs/security.md +++ b/docs/security.md @@ -22,7 +22,7 @@ With a subscription token and no API key, Claude uses `--safe-mode` instead of ` `--bare` does not read OAuth credentials. Safe mode skips repository CLAUDE.md files, hooks, plugins, skills, and MCP servers. A local test with a hostile CLAUDE.md and project hooks showed no hook run in safe mode. -Codex uses `exec --sandbox read-only`, an ephemeral session, and ignores user config and rules. +Codex uses `exec --sandbox read-only --json`, an ephemeral session, and ignores user config and rules. OpenCode denies all tools except read, glob, grep, and the two worker-supplied OSV lookup tools. It also uses `--pure` and disables project config, Claude config, and external skill discovery. These controls stop PR files from supplying OpenCode plugins or startup config. @@ -176,6 +176,21 @@ Selecting a Docker network does not create these firewall or proxy rules. `runsc` adds isolation. It does not create a provider egress allowlist. A `none` network also blocks model access unless you provide a supported local route. +The Claude model list run receives the Claude credentials, like the OpenCode list run. +It calls the Anthropic Models API, or `ANTHROPIC_BASE_URL` when set, and rejects redirects. +Model IDs that contain a known secret value are dropped. Codex list runs receive no credentials. + +The weekly security digest is opt-in. It sends repository names, package names, and +advisory IDs from the worker to the webhook host that an admin chose. That host and +its chat service receive this data. +The webhook URL works like a password. The database stores it in plain text. +The API never returns it, and error messages never contain it. +The worker sends one HTTPS request to port 443 and follows no redirects. +It resolves the host once and refuses the send when any address is private, loopback, +link-local, shared, reserved, or otherwise not public. It then connects to a checked address. +The API also rejects IP literals, credentials in the URL, and local host names. +These checks reduce server-side request forgery. They do not replace an egress firewall. + The database stores summaries, findings, PR metadata, and redacted errors. The worker removes completed reviews 30 days after completion. For legacy rows with no completion time, it uses the creation time. diff --git a/migrations/0004_usage_health_digest.sql b/migrations/0004_usage_health_digest.sql new file mode 100644 index 0000000..612db77 --- /dev/null +++ b/migrations/0004_usage_health_digest.sql @@ -0,0 +1,24 @@ +-- Agent run time and the token usage that the agent CLI reports. Null when +-- the review has no agent run or the CLI reports no usage. +ALTER TABLE reviews + ADD COLUMN agent_duration_ms BIGINT, + ADD COLUMN input_tokens BIGINT, + ADD COLUMN cached_input_tokens BIGINT, + ADD COLUMN output_tokens BIGINT; + +-- Result of the latest model list run. A failed run keeps the old models. +ALTER TABLE agent_models + ADD COLUMN available BOOLEAN NOT NULL DEFAULT TRUE, + ADD COLUMN checked_at TIMESTAMPTZ; +UPDATE agent_models SET checked_at = detected_at; +ALTER TABLE agent_models + ALTER COLUMN checked_at SET NOT NULL, + ALTER COLUMN checked_at SET DEFAULT now(); + +-- Opt-in weekly security digest. The webhook URL is a secret: the API never +-- returns it. +ALTER TABLE installations + ADD COLUMN digest_webhook_url TEXT, + ADD COLUMN digest_sent_at TIMESTAMPTZ, + ADD COLUMN digest_attempted_at TIMESTAMPTZ, + ADD COLUMN digest_error TEXT; diff --git a/web/app/(dashboard)/installations/[id]/actions.ts b/web/app/(dashboard)/installations/[id]/actions.ts index 522320a..4e64257 100644 --- a/web/app/(dashboard)/installations/[id]/actions.ts +++ b/web/app/(dashboard)/installations/[id]/actions.ts @@ -25,12 +25,21 @@ export async function saveSettings(id: string, form: FormData): Promise { ) failure("agent"); if (typeof model !== "string") failure("model"); + // Undefined keeps the stored URL: the API never returns it to the form. + const url = form.get("digest_webhook_url"); + const newUrl = typeof url === "string" ? url.trim() : ""; + let digest: string | null | undefined; + if (form.get("digest") !== "on") + digest = installation.digest.enabled ? null : undefined; + else if (newUrl) digest = newUrl; + else if (!installation.digest.enabled) failure("digest_url"); await api(`/api/v1/installations/${id}/settings`, { method: "PATCH", body: { enabled: form.get("enabled") === "on", agent: agent === DEFAULT_AGENT ? null : agent, model: model.trim() || null, + ...(digest === undefined ? {} : { digest_webhook_url: digest }), }, }); revalidatePath(target); diff --git a/web/app/(dashboard)/installations/[id]/page.tsx b/web/app/(dashboard)/installations/[id]/page.tsx index 726200b..f9c3715 100644 --- a/web/app/(dashboard)/installations/[id]/page.tsx +++ b/web/app/(dashboard)/installations/[id]/page.tsx @@ -3,6 +3,7 @@ import type { Metadata } from "next"; import Link from "next/link"; import { notFound } from "next/navigation"; import { ActivityChart } from "@/components/activity-chart"; +import { AgentHealthTable } from "@/components/agent-health"; import { FindingsBreakdown } from "@/components/findings-breakdown"; import { OpenPullsTable } from "@/components/open-pulls-table"; import { StatGrid } from "@/components/page-header"; @@ -30,6 +31,7 @@ import { Meter } from "@/components/ui/meter"; import { api, optionalApi } from "@/lib/api"; import { formatSeconds, validCursor, validInstallationId } from "@/lib/format"; import { + type Agents, type Installation, type OpenPulls, type Review, @@ -59,11 +61,12 @@ export default async function InstallationPage({ limit: String(PAGE_SIZE), ...(before ? { before } : {}), }); - const [installation, reviews, stats, pulls] = await Promise.all([ + const [installation, reviews, stats, pulls, agents] = await Promise.all([ api(`/api/v1/installations/${id}`), api(`/api/v1/installations/${id}/reviews?${query}`), api(`/api/v1/installations/${id}/stats`), optionalApi(`/api/v1/installations/${id}/pulls`), + optionalApi("/api/v1/agents"), ]); const webUrl = ( process.env.GITHUB_WEB_URL?.trim() || "https://github.com" @@ -219,6 +222,25 @@ export default async function InstallationPage({ + + + } className="text-sm"> + Agent health and usage + + + Harness shows whether the agent CLI answered its last model list + check in the sandbox. It does not test credentials. Time and tokens + cover this installation for the last {stats.days} days, as each + CLI reports them. + + + + + } className="text-sm"> diff --git a/web/app/(dashboard)/installations/[id]/settings/page.tsx b/web/app/(dashboard)/installations/[id]/settings/page.tsx index aa76e9c..d5d990a 100644 --- a/web/app/(dashboard)/installations/[id]/settings/page.tsx +++ b/web/app/(dashboard)/installations/[id]/settings/page.tsx @@ -12,8 +12,9 @@ const errors: Record = { forbidden: "Only an account owner or organization admin can change settings.", agent: "Select an available agent.", model: "Enter a model, or leave the field empty.", + digest_url: "Enter a webhook URL to turn on the weekly security digest.", validation: - "The API did not accept these settings. Check the agent and model.", + "The API did not accept these settings. Check the agent, the model, and that the webhook URL uses https and a public host name.", api: "Cannot save settings. Check that the CroCode API is available, then try again.", }; diff --git a/web/app/(dashboard)/reviews/[id]/actions.ts b/web/app/(dashboard)/reviews/[id]/actions.ts new file mode 100644 index 0000000..4dc5325 --- /dev/null +++ b/web/app/(dashboard)/reviews/[id]/actions.ts @@ -0,0 +1,32 @@ +"use server"; + +import { revalidatePath } from "next/cache"; +import { notFound, redirect } from "next/navigation"; +import { api } from "@/lib/api"; +import { ApiError } from "@/lib/api-client"; +import { validReviewId } from "@/lib/format"; +import type { Review } from "@/lib/types"; + +async function control(id: string, command: "retry" | "cancel"): Promise { + if (!validReviewId(id)) notFound(); + const target = `/reviews/${id}`; + try { + await api(`/api/v1/reviews/${id}/${command}`, { method: "POST" }); + } catch (error) { + if (error instanceof ApiError) + redirect( + `${target}?error=${error.status === 403 ? "forbidden" : error.status === 409 ? "conflict" : "api"}`, + ); + throw error; + } + revalidatePath(target); + redirect(`${target}?done=${command}`); +} + +export async function retryReview(id: string): Promise { + return control(id, "retry"); +} + +export async function cancelReview(id: string): Promise { + return control(id, "cancel"); +} diff --git a/web/app/(dashboard)/reviews/[id]/page.tsx b/web/app/(dashboard)/reviews/[id]/page.tsx index 15e264d..c4638d9 100644 --- a/web/app/(dashboard)/reviews/[id]/page.tsx +++ b/web/app/(dashboard)/reviews/[id]/page.tsx @@ -4,6 +4,7 @@ import Link from "next/link"; import { notFound } from "next/navigation"; import { Avatar } from "@/components/brand"; import { PageHeader } from "@/components/page-header"; +import { ReviewControls } from "@/components/review-controls"; import { ReviewFindings } from "@/components/review-findings"; import { severityColor, StatusBadge } from "@/components/status-badge"; import { TimeAgo } from "@/components/time-ago"; @@ -13,7 +14,14 @@ import { Button } from "@/components/ui/button"; import { Tabs, TabsList, TabsPanel, TabsTab } from "@/components/ui/tabs"; import { api, optionalApi } from "@/lib/api"; import { accountAvatar } from "@/lib/avatar"; -import { formatDuration, formatTime, validReviewId } from "@/lib/format"; +import { canCancel, canRetry, explainReview } from "@/lib/failure"; +import { + formatCount, + formatDuration, + formatSeconds, + formatTime, + validReviewId, +} from "@/lib/format"; import { type Installation, type PullDetail, @@ -35,12 +43,27 @@ const pullState: Record< closed: { label: "Closed", variant: "secondary" }, }; +const notices: Record = { + retry: "The review is queued again. Reload the page to follow it.", + cancel: + "The review is cancelled. A running agent stops within about 20 seconds.", +}; +const errors: Record = { + forbidden: "Only an account owner or organization admin can control reviews.", + conflict: + "The review changed state, or a newer review of this pull request exists. Reload the page.", + api: "Cannot reach the CroCode API. Try again.", +}; + export default async function ReviewPage({ params, + searchParams, }: { params: Promise<{ id: string }>; + searchParams: Promise<{ done?: string; error?: string }>; }) { const { id } = await params; + const { done, error } = await searchParams; if (!validReviewId(id)) notFound(); const { review, @@ -71,10 +94,33 @@ export default async function ReviewPage({ const state = pull ? pullState[pull.state === "open" && pull.draft ? "draft" : pull.state] : null; + const failure = explainReview(review); + // Only the newest review of a pull request can run again. + const newest = timeline[0]?.id === review.id; + const showControls = + installation.can_admin && + (canCancel(review.status) || (canRetry(review.status) && newest)); + const tokens = + review.input_tokens === null && review.output_tokens === null + ? "Not reported" + : `${formatCount(review.input_tokens) ?? "–"} in · ${formatCount(review.output_tokens) ?? "–"} out`; const details = [ ["Agent", review.agent ?? "Not picked yet"], ["Model", review.model ?? "Agent default"], ["Duration", formatDuration(review.created_at, review.finished_at) ?? "–"], + [ + "Agent time", + formatSeconds( + review.agent_duration_ms === null + ? null + : review.agent_duration_ms / 1000, + ) ?? "–", + ], + ["Tokens", tokens], + [ + "Cached input", + formatCount(review.cached_input_tokens) ?? "Not reported", + ], ["Attempts", String(review.attempts)], ["Queued", formatTime(review.created_at)], [ @@ -114,13 +160,33 @@ export default async function ReviewPage({ } actions={ - + <> + {showControls && ( + + )} + + } /> + {error && errors[error] && ( + + {errors[error]} + + )} + {!error && done && notices[done] && ( + + {notices[done]} + + )} + {pull && (

)} - {review.error && ( - + {failure && ( + - Review error - {/* Agent output is untrusted: render it as plain text, never as HTML. */} - - {review.error} + {failure.title} + +

+ {failure.detail} + {failure.action && ` ${failure.action}`} +

+ {review.error && ( +
+ + Error details + + {/* Agent output is untrusted: render it as plain text, never as HTML. */} +

+ {review.error} +

+
+ )} )} @@ -290,12 +369,16 @@ export default async function ReviewPage({ {formatDuration(run.created_at, run.finished_at) && ` · ${formatDuration(run.created_at, run.finished_at)}`}

- {run.error && ( + {explainReview(run) && (

- {run.error} + {explainReview(run)?.title}

)} diff --git a/web/components/agent-health.tsx b/web/components/agent-health.tsx new file mode 100644 index 0000000..9efde3f --- /dev/null +++ b/web/components/agent-health.tsx @@ -0,0 +1,134 @@ +import { TimeAgo } from "@/components/time-ago"; +import { Badge } from "@/components/ui/badge"; +import { + Table, + TableBody, + TableCell, + TableHead, + TableHeader, + TableRow, +} from "@/components/ui/table"; +import { formatCount, formatSeconds } from "@/lib/format"; +import type { AgentHealth, AgentUsage, Agents } from "@/lib/types"; + +/** Enabled agents first, then agents that only appear in review history. */ +export function agentRows(agents: Agents | null, usage: AgentUsage[]) { + const names = [ + ...(agents?.agents ?? []), + ...usage.map((u) => u.name).filter((n) => !agents?.agents.includes(n)), + ]; + return names.map((name) => ({ + name, + enabled: agents?.agents.includes(name) ?? null, + health: agents?.health?.[name] ?? null, + usage: usage.find((u) => u.name === name) ?? null, + })); +} + +function Harness({ + enabled, + health, +}: { + enabled: boolean | null; + health: AgentHealth | null; +}) { + if (enabled === false) return Not enabled; + if (!health) + return ( + {enabled === null ? "Unknown" : "Not checked"} + ); + return ( +
+ + {health.available ? "Available" : "Unavailable"} + + + Checked + +
+ ); +} + +export function AgentHealthTable({ + agents, + usage, + days, +}: { + agents: Agents | null; + usage: AgentUsage[]; + days: number; +}) { + const rows = agentRows(agents, usage); + if (rows.length === 0) + return ( +

+ No agents are enabled on this server. +

+ ); + return ( + + + + Agent + Harness + Reviews, {days} days + Median agent time + Tokens + Models + + + + {rows.map(({ name, enabled, health, usage }) => ( + + + {name} + {agents?.default === name && ( + + default + + )} + + + + + + {usage ? ( + <> + {usage.reviews} + + {usage.succeeded} succeeded · {usage.failed} failed + + + ) : ( + "0" + )} + + + {formatSeconds(usage?.median_agent_secs ?? null) ?? "–"} + + + {usage && usage.usage_reviews > 0 ? ( + <> + + {formatCount(usage.input_tokens)} in ·{" "} + {formatCount(usage.output_tokens)} out + + + {formatCount(usage.cached_input_tokens)} cached · from{" "} + {usage.usage_reviews} of {usage.reviews}{" "} + {usage.reviews === 1 ? "review" : "reviews"} + + + ) : ( + Not reported + )} + + + {usage?.models.length ? usage.models.join(", ") : "–"} + + + ))} + +
+ ); +} diff --git a/web/components/review-controls.tsx b/web/components/review-controls.tsx new file mode 100644 index 0000000..8e87e05 --- /dev/null +++ b/web/components/review-controls.tsx @@ -0,0 +1,51 @@ +"use client"; + +import { RotateCcw, Square } from "lucide-react"; +import { useFormStatus } from "react-dom"; +import { + cancelReview, + retryReview, +} from "@/app/(dashboard)/reviews/[id]/actions"; +import { Button } from "@/components/ui/button"; + +/** Admin controls. The API checks access and state again. */ +export function ReviewControls({ + id, + retry, + cancel, +}: { + id: string; + retry: boolean; + cancel: boolean; +}) { + return ( + <> + {cancel && ( +
+ +
+ )} + {retry && ( +
+ +
+ )} + + ); +} + +function Submit({ + icon, + children, +}: { + icon: React.ReactNode; + children: React.ReactNode; +}) { + const { pending } = useFormStatus(); + return ( + + ); +} diff --git a/web/components/settings-form.tsx b/web/components/settings-form.tsx index 73160d2..568baf9 100644 --- a/web/components/settings-form.tsx +++ b/web/components/settings-form.tsx @@ -27,7 +27,9 @@ import { SelectTrigger, SelectValue, } from "@/components/ui/select"; +import { Input } from "@/components/ui/input"; import { Switch } from "@/components/ui/switch"; +import { formatTime } from "@/lib/format"; import { DEFAULT_AGENT, type Agents, type Installation } from "@/lib/types"; type ModelItem = { value: string; label: string; name: string }; @@ -53,6 +55,8 @@ export function SettingsForm({ const [agent, setAgent] = useState(saved); const [model, setModel] = useState(installation.settings.model ?? ""); + const digest = installation.digest; + const [digestOn, setDigestOn] = useState(digest.enabled); const effective = agent === DEFAULT_AGENT ? agents.default : agent; const models: ModelItem[] = (agents.models[effective] ?? []).map((m) => ({ value: m.id, @@ -192,6 +196,80 @@ export function SettingsForm({

+
+ Weekly security digest +
+
+ +

+ Every 7 days, CroCode posts new Dependabot alerts, fixed + alerts, and open alerts past their deadline to a chat webhook. + The message leaves GitHub. It names repositories, packages, + and advisories. +

+
+ +
+ {digest.enabled && ( +

+ Sends to{" "} + {digest.webhook_host}.{" "} + {digest.sent_at + ? `Last sent ${formatTime(digest.sent_at)}.` + : "Not sent yet. The first digest goes out within about 10 minutes."} + {digest.error && ( + + Last attempt failed: {digest.error}. CroCode tries again + every hour. + + )} +

+ )} + {digestOn && ( +
+ + +

+ {digest.enabled + ? "Leave empty to keep the current URL. " + : ""} + Use an HTTPS Slack or Mattermost incoming webhook, or a + Discord webhook URL that ends in /slack. CroCode stores the URL + as a secret and never shows it again. +

+
+ )} +
diff --git a/web/lib/api-client.ts b/web/lib/api-client.ts index 14ff45c..0f1b889 100644 --- a/web/lib/api-client.ts +++ b/web/lib/api-client.ts @@ -7,9 +7,11 @@ export class ApiError extends Error { ? "You do not have access to this resource." : status === 404 ? "This resource was not found." - : status === 422 - ? "The API did not accept these settings. Check the agent and model." - : status === 0 + : status === 409 + ? "The review changed state. Reload the page and try again." + : status === 422 + ? "The API did not accept these settings. Check the agent, model, and webhook URL." + : status === 0 ? "Cannot connect to the CroCode API. Try again." : "The CroCode API could not complete this request. Try again.", ); @@ -21,7 +23,7 @@ export async function apiRequest( baseUrl: string, token: string, path: string, - options: { method?: "GET" | "PATCH"; body?: unknown } = {}, + options: { method?: "GET" | "PATCH" | "POST"; body?: unknown } = {}, fetcher: typeof fetch = fetch, ): Promise { if ( diff --git a/web/lib/api.ts b/web/lib/api.ts index 082f355..6bc9030 100644 --- a/web/lib/api.ts +++ b/web/lib/api.ts @@ -6,7 +6,7 @@ import { getEnv } from "./env"; export async function api( path: string, - options?: { method?: "GET" | "PATCH"; body?: unknown }, + options?: { method?: "GET" | "PATCH" | "POST"; body?: unknown }, ): Promise { const token = await requireAccessToken(); try { diff --git a/web/lib/docs-content.tsx b/web/lib/docs-content.tsx index 9821cc7..1a2468b 100644 --- a/web/lib/docs-content.tsx +++ b/web/lib/docs-content.tsx @@ -268,6 +268,18 @@ export const DOCS_CONTENT: Record = {

These values are code constants. See the full configuration reference for Compose and retention details.

, }, + { + id: "digest", title: "Weekly security digest", + content: <> +

The weekly security digest is off by default. An installation admin turns it on in Settings with an HTTPS chat webhook URL. CroCode posts a Slack-compatible {'{"text": "..."}'} body. Slack and Mattermost accept it. For Discord, add /slack to the end of the webhook URL.

+

Every 7 days, the worker reads the Dependabot alerts of all installation repositories with a token that can only read alerts and metadata. The digest counts new alerts, fixed alerts, and open alerts past their deadline. It lists at most 5 alerts in each section.

+ critical, "15 days"], [high, "30 days"], + [medium, "90 days"], [low, "180 days"], + ]} /> +

The message names repositories, packages, and CVE or GHSA IDs. It leaves GitHub. The API never returns the webhook URL. The worker sends only to public addresses on port 443 and follows no redirects. After a failed send it tries again every hour.

+ , + }, ], "/docs/configuration/agents": [ { @@ -335,10 +347,12 @@ export const DOCS_CONTENT: Record = {
  • Turn reviews on or off.
  • Choose an enabled agent.
  • Choose a model from the list, or type a model name. Leave the model blank to use the agent default.
    • +
  • Optional: turn on the Weekly security digest and enter an HTTPS chat webhook URL.
  • Save the settings.

    • The model list shows the models that each agent reports. For Claude, it shows the aliases and the versioned models from the Anthropic Models API. The worker checks them at start and every 6 hours.

    The Overview tab shows open pull requests and the state of their newest review. It also shows review totals, reviews per day, and findings by severity for the last 30 days.

    +

    The Agent health and usage card shows whether each agent CLI answered its last model list check in the sandbox. This check does not test credentials. The card also shows reviews, median agent time, token counts, and models for each agent. Token counts appear only when the agent CLI reports them.

    Only the account owner or an active organization admin can change settings. The server checks this access again when you save. Repository config can override the installation’s agent and model.

    , }, @@ -352,7 +366,9 @@ export const DOCS_CONTENT: Record = { ["cancelled", "The job was cancelled, for example after a new push."], ["skipped", "Review policy skipped the job."], ]} /> -

    Check the error text for failed reviews. The current API has no manual rerun endpoint. A head commit that already has a queue row is not queued again.

    +

    A failed or cancelled review shows a plain reason and a next step. Open Error details to read the stored error.

    +

    An admin can select Cancel review on a queued or running review. A running agent stops within about 20 seconds. An admin can select Retry review on a failed or cancelled review when it is the newest review of its pull request. A retry starts with fresh attempts and does not post a GitHub review twice.

    +

    The Summary tab also shows agent time and the token counts that the agent CLI reported.

    , }, { @@ -437,7 +453,7 @@ export const DOCS_CONTENT: Record = { content:
    /healthz, "No authentication. Returns ok."], ["GET", /api/v1/me, "Signed-in user ID and login."], - ["GET", /api/v1/agents, "Enabled agents, server default, and detected models."], + ["GET", /api/v1/agents, "Enabled agents, server default, detected models, and harness health."], ["GET", /api/v1/installations, "Accessible installations that exist in the database."], ["GET", /api/v1/installations/{"{id}"}, "One accessible installation."], ["PATCH", /api/v1/installations/{"{id}"}/settings, "Update settings. Requires can_admin."], @@ -446,14 +462,16 @@ export const DOCS_CONTENT: Record = { ["GET", /api/v1/installations/{"{id}"}/pulls, "Open pull requests with their newest review."], ["GET", /api/v1/reviews/{"{id}"}, "One review with its summary and findings."], ["GET", /api/v1/reviews/{"{id}"}/pull, "Live pull request details from GitHub."], + ["POST", /api/v1/reviews/{"{id}"}/retry, "Queue the newest failed or cancelled review again. Requires can_admin."], + ["POST", /api/v1/reviews/{"{id}"}/cancel, "Cancel a queued or running review. Requires can_admin."], ]} />, }, { id: "installations", title: "Installations and settings", content: <> - +

    can_admin is true for the account owner or an organization admin. A null agent uses the server default. A null model uses the selected agent’s default.

    -

    All fields in the settings PATCH body are optional. The response is the updated installation.

    +

    All fields in the settings PATCH body are optional. The response is the updated installation. Set digest_webhook_url to an HTTPS URL to turn on the weekly security digest, or to null to turn it off. The API never returns the URL.

    , }, @@ -462,8 +480,8 @@ export const DOCS_CONTENT: Record = { content: <>

    Use limit from 1 to 100. The default is 50. Supply an RFC 3339 timestamp as before for older reviews. Add repo and pr together to list the reviews of one pull request.

    - -

    Status is queued, running, succeeded, failed, cancelled, or skipped. The agent is null until a worker selects one.

    + +

    Status is queued, running, succeeded, failed, cancelled, or skipped. The agent is null until a worker selects one. Token fields are null when the agent CLI reports no usage.

    , }, { diff --git a/web/lib/failure.test.ts b/web/lib/failure.test.ts new file mode 100644 index 0000000..e1b1c11 --- /dev/null +++ b/web/lib/failure.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, test } from "bun:test"; +import { canCancel, canRetry, explainReview } from "./failure"; + +const title = (status: "failed" | "cancelled" | "succeeded", error: string | null) => + explainReview({ status, error })?.title; + +describe("explainReview", () => { + test("maps stored worker errors to plain reasons", () => { + for (const [error, expected] of [ + ["agent timed out after 900s", "The agent took too long"], + ["worker lost", "The worker stopped during the review"], + ["codex error: You've hit your usage limit", "The agent reached a usage limit"], + [ + "claude returned no structured output: Not logged in (exit ok: false; stderr: )", + "The agent could not sign in", + ], + [ + "agent JSON does not match the review schema (exit ok: true; stderr: )", + "The agent returned no valid review", + ], + ["start agent process: No such file or directory", "The sandbox could not start"], + ["git fetch failed: fatal: couldn't find remote ref", "The pull request could not be fetched"], + ["create installation token: GitHub", "A GitHub request failed"], + ["invalid .crocode.yml: unknown field", "The repository config is not valid"], + ["agent opencode is not enabled on this server", "The agent or model is not available"], + ["something new", "The review failed"], + [null, "The review failed"], + ] as const) + expect(title("failed", error)).toBe(expected); + }); + + test("explains each cancel reason", () => { + expect(title("cancelled", "superseded")).toBe("A newer commit replaced this review"); + expect(title("cancelled", "cancelled from the dashboard")).toBe( + "An admin cancelled this review", + ); + expect(title("cancelled", null)).toBe("The review was cancelled"); + expect(explainReview({ status: "cancelled", error: "odd" })?.detail).toBe("odd"); + }); + + test("has no reason for other states", () => { + expect(title("succeeded", "ignored")).toBeUndefined(); + }); + + test("allows controls only in matching states", () => { + expect([canRetry("failed"), canRetry("cancelled"), canRetry("running")]).toEqual([ + true, + true, + false, + ]); + expect([canCancel("queued"), canCancel("running"), canCancel("failed")]).toEqual([ + true, + true, + false, + ]); + }); +}); diff --git a/web/lib/failure.ts b/web/lib/failure.ts new file mode 100644 index 0000000..3ba140f --- /dev/null +++ b/web/lib/failure.ts @@ -0,0 +1,164 @@ +import type { Review } from "./types"; + +export type FailureReason = { + title: string; + /** What happened, in plain words. */ + detail: string; + /** What to do next. Empty when no action helps. */ + action: string; +}; + +/** Patterns match the messages that the worker and webhook store. */ +const failed: [RegExp, FailureReason][] = [ + [ + /usage limit|rate.?limit|insufficient_quota|you've hit your|free usage limit/i, + { + title: "The agent reached a usage limit", + detail: "The model provider refused more requests for now.", + action: + "Wait until the limit resets, then retry. Set CROCODE_FALLBACK_AGENT to switch agents automatically.", + }, + ], + [ + /not logged in|invalid api key|authentication|unauthori[sz]ed|oauth|credential|login/i, + { + title: "The agent could not sign in", + detail: "The agent CLI rejected its credentials.", + action: + "Check the agent key or subscription token on the worker, then retry.", + }, + ], + [ + /agent timed out/i, + { + title: "The agent took too long", + detail: "The agent did not finish before the worker timeout.", + action: + "Retry. If it fails again, raise CROCODE_AGENT_TIMEOUT_SECS or split the pull request.", + }, + ], + [ + /^worker lost$/, + { + title: "The worker stopped during the review", + detail: + "A worker crashed or restarted and stopped sending heartbeats too many times.", + action: "Check the worker logs, then retry.", + }, + ], + [ + /structured output|no JSON object|review schema|no final message|codex error|opencode error/i, + { + title: "The agent returned no valid review", + detail: "The agent output did not match the review format.", + action: "Retry. If it fails again, choose another model.", + }, + ], + [ + /start agent process|docker|sandbox/i, + { + title: "The sandbox could not start", + detail: "The worker could not run the agent container.", + action: + "Check Docker access, DOCKER_GID, and the sandbox image on the worker, then retry.", + }, + ], + [ + /^git .*(failed|timed out)|run git/i, + { + title: "The pull request could not be fetched", + detail: "Git could not download the pull request commit.", + action: "Check that the commit still exists, then retry.", + }, + ], + [ + /installation token|GitHub|status code|HTTP \d{3}/i, + { + title: "A GitHub request failed", + detail: "GitHub refused or could not answer a request for this review.", + action: + "Check that the app can still access the repository, then retry.", + }, + ], + [ + /^invalid \.crocode\.yml/i, + { + title: "The repository config is not valid", + detail: "CroCode reads .crocode.yml from the base branch.", + action: "Fix .crocode.yml on the base branch, then retry.", + }, + ], + [ + /is not enabled on this server|unknown agent|invalid model name/i, + { + title: "The agent or model is not available", + detail: "The selected agent or model cannot run on this server.", + action: + "Choose an enabled agent and a valid model in Settings or .crocode.yml, then retry.", + }, + ], + [ + /^installation not found$/, + { + title: "The installation is gone", + detail: "The GitHub App installation no longer exists.", + action: "", + }, + ], +]; + +const cancelled: Record = { + superseded: { + title: "A newer commit replaced this review", + detail: "CroCode reviews only the newest commit of a pull request.", + action: "", + }, + "pull request closed": { + title: "The pull request was closed", + detail: "CroCode stops reviews of closed pull requests.", + action: "", + }, + "pull request converted to draft": { + title: "The pull request became a draft", + detail: "CroCode stops reviews of drafts unless review_drafts is on.", + action: "", + }, + "installation removed": { + title: "The app was suspended or removed", + detail: "The GitHub App installation stopped while this review waited.", + action: "", + }, + "cancelled from the dashboard": { + title: "An admin cancelled this review", + detail: "Someone cancelled the review from the dashboard.", + action: "", + }, +}; + +/** Plain-language reason for a failed or cancelled review. Null otherwise. */ +export function explainReview( + review: Pick, +): FailureReason | null { + const error = review.error?.trim() ?? ""; + if (review.status === "cancelled") + return ( + cancelled[error] ?? { + title: "The review was cancelled", + detail: error || "The review stopped before it finished.", + action: "", + } + ); + if (review.status !== "failed") return null; + return ( + failed.find(([pattern]) => pattern.test(error))?.[1] ?? { + title: "The review failed", + detail: "The worker stopped with an unexpected error.", + action: "See the error details. Retry if the cause was temporary.", + } + ); +} + +export const canRetry = (status: Review["status"]) => + status === "failed" || status === "cancelled"; +export const canCancel = (status: Review["status"]) => + status === "queued" || status === "running"; diff --git a/web/lib/format.test.ts b/web/lib/format.test.ts index 69dccf9..95b8fae 100644 --- a/web/lib/format.test.ts +++ b/web/lib/format.test.ts @@ -1,5 +1,18 @@ import { describe, expect, test } from "bun:test"; -import { formatDuration, formatRelative, formatSeconds } from "./format"; +import { + formatCount, + formatDuration, + formatRelative, + formatSeconds, +} from "./format"; + +test("formatCount stays compact and keeps unknown values", () => { + expect(formatCount(0)).toBe("0"); + expect(formatCount(950)).toBe("950"); + expect(formatCount(24_388)).toBe("24.4K"); + expect(formatCount(1_250_000)).toBe("1.3M"); + expect(formatCount(null)).toBeNull(); +}); const now = Date.parse("2026-11-01T14:00:00Z"); diff --git a/web/lib/format.ts b/web/lib/format.ts index 82d3780..c9c82e9 100644 --- a/web/lib/format.ts +++ b/web/lib/format.ts @@ -55,6 +55,15 @@ export function formatDuration(start: string, end: string | null) { } /** "2m 41s" for a number of seconds, or null when it is not valid. */ +/** Compact count, such as `12.3K`. Null stays null. */ +export function formatCount(value: number | null) { + if (value === null || !Number.isFinite(value)) return null; + return new Intl.NumberFormat("en", { + notation: "compact", + maximumFractionDigits: 1, + }).format(value); +} + export function formatSeconds(value: number | null) { if (value === null) return null; const seconds = Math.round(value); diff --git a/web/lib/types.ts b/web/lib/types.ts index ad22288..56c186c 100644 --- a/web/lib/types.ts +++ b/web/lib/types.ts @@ -1,10 +1,14 @@ export type User = { id: number; login: string }; export type AgentModel = { id: string; name: string }; +/** Result of the latest model list run in the sandbox. */ +export type AgentHealth = { available: boolean; checked_at: string }; export type Agents = { agents: string[]; default: string; /** Models each enabled agent CLI reports. Empty until the worker detects them. */ models: Record; + /** Null until the worker checks the agent. */ + health: Record; }; /** Form value for "use the server default agent". The API stores it as null. */ export const DEFAULT_AGENT = "default"; @@ -24,8 +28,17 @@ export type Installation = { suspended: boolean; created_at: string; settings: Settings; + digest: Digest; can_admin: boolean; }; +/** Opt-in weekly security digest. The API never returns the webhook URL. */ +export type Digest = { + enabled: boolean; + webhook_host: string | null; + sent_at: string | null; + attempted_at: string | null; + error: string | null; +}; export type ReviewStatus = | "queued" | "running" @@ -49,6 +62,12 @@ export type Review = { error: string | null; created_at: string; finished_at: string | null; + /** Time in agent processes. Null when no agent finished. */ + agent_duration_ms: number | null; + /** Token counts that the agent CLI reported. Input includes cached input. */ + input_tokens: number | null; + cached_input_tokens: number | null; + output_tokens: number | null; }; export const SEVERITIES = [ "critical", @@ -98,6 +117,21 @@ export type Stats = { categories: NameCount[]; /** One entry per UTC day, oldest first. */ daily: { date: string; reviews: number; failed: number }[]; + /** Reviews with a picked agent, most used first. */ + agents: AgentUsage[]; +}; +export type AgentUsage = { + name: string; + reviews: number; + succeeded: number; + failed: number; + median_agent_secs: number | null; + /** Reviews that reported token usage. Token sums cover only these. */ + usage_reviews: number; + input_tokens: number; + cached_input_tokens: number; + output_tokens: number; + models: string[]; }; export type OpenPull = { repo_full_name: string; diff --git a/web/scripts/smoke.ts b/web/scripts/smoke.ts index 7c303ea..7b1e0d9 100644 --- a/web/scripts/smoke.ts +++ b/web/scripts/smoke.ts @@ -16,6 +16,13 @@ const installation = { suspended: false, created_at: "2026-11-01T10:00:00Z", settings: { enabled: true, agent: "codex", model: null }, + digest: { + enabled: false, + webhook_host: null, + sent_at: null, + attempted_at: null, + error: null, + }, can_admin: true, }; const review = { @@ -30,10 +37,15 @@ const review = { model: null, attempts: 1, finding_count: 2, - error: null, + error: null as string | null, created_at: "2026-11-01T10:00:00.123456Z", finished_at: "2026-11-01T10:03:00Z", + agent_duration_ms: 95_000, + input_tokens: 24_388, + cached_input_tokens: 12_288, + output_tokens: 512, }; +const controls: string[] = []; let apiStatus = 200; let canAdmin = true; let empty = false; @@ -60,7 +72,18 @@ const mock = Bun.serve({ codex: [{ id: "gpt-smoke", name: "GPT Smoke" }], claude: [{ id: "opus", name: "opus" }], }, + health: { + codex: { available: true, checked_at: "2026-11-01T10:00:00Z" }, + claude: { available: false, checked_at: "2026-11-01T10:00:00Z" }, + }, }); + if ( + request.method === "POST" && + /^\/api\/v1\/reviews\/[^/]+\/(retry|cancel)$/.test(url.pathname) + ) { + controls.push(url.pathname); + return Response.json({ ...review, status: "queued" }); + } if (url.pathname.endsWith("/settings") && request.method === "PATCH") { const body: unknown = await request.json(); patches.push(body); @@ -115,6 +138,20 @@ const mock = Bun.serve({ reviews: i === 29 ? 20 : 0, failed: i === 29 ? 1 : 0, })), + agents: [ + { + name: "codex", + reviews: 20, + succeeded: 19, + failed: 1, + median_agent_secs: 95, + usage_reviews: 19, + input_tokens: 1_250_000, + cached_input_tokens: 600_000, + output_tokens: 40_000, + models: ["gpt-smoke"], + }, + ], }); if (url.pathname === "/api/v1/installations/123/pulls") { if (pullsFail) return Response.json({ error: "GitHub" }, { status: 502 }); @@ -389,6 +426,9 @@ try { assert.equal(response.status, 200); assert.match(overview, /Load older/); assert.match(overview, /Not reviewed/); + assert.match(overview, /Agent health and usage/); + assert.match(overview, /Unavailable/); + assert.match(overview, /1\.3M()? in ·()? ()?40K()? out/); assert.ok(overview.includes("<b>Open pull request</b>")); assert.ok(!overview.includes("Open pull request")); assert.ok(!overview.includes('name="agent"'), "Settings live in their own tab"); @@ -455,7 +495,7 @@ try { response = await get("/installations/123/settings"); const settings = await response.text(); assert.equal(response.status, 200); - assert.match(settings, /1 model detected from the codex CLI/); + assert.match(settings, /1 model detected for codex/); assert.match(settings, /]*name="model"/, "Model field submits as model"); assert.match(settings, /]*name="agent"/, "Agent field submits as agent"); const form = actionForm(settings, 'name="agent"'); @@ -475,6 +515,36 @@ try { "/installations/123/settings?saved=1", ); assert.deepEqual(patches, [{ enabled: false, agent: "codex", model: null }]); + assert.match(settings, /Weekly security digest/); + form.set("digest", "on"); + const missingUrl = await fetch(origin + "/installations/123/settings", { + method: "POST", + headers: { Cookie: sessionCookie, Origin: origin }, + body: form, + redirect: "manual", + signal: AbortSignal.timeout(15_000), + }); + assert.equal( + missingUrl.headers.get("location"), + "/installations/123/settings?error=digest_url", + ); + form.set("digest_webhook_url", " https://hooks.slack.com/services/T/B/x "); + await fetch(origin + "/installations/123/settings", { + method: "POST", + headers: { Cookie: sessionCookie, Origin: origin }, + body: form, + redirect: "manual", + signal: AbortSignal.timeout(15_000), + }); + assert.deepEqual(patches.at(-1), { + enabled: false, + agent: "codex", + model: null, + digest_webhook_url: "https://hooks.slack.com/services/T/B/x", + }); + patches.length = 1; + form.delete("digest"); + form.delete("digest_webhook_url"); assert.match( await (await get("/installations/123/settings?saved=1")).text(), /Settings saved/, @@ -525,6 +595,26 @@ try { assert.ok(!detail.includes('')); assert.ok(detail.includes("<script>")); assert.ok(!detail.includes('')); + assert.match(detail, /24\.4K in · 512 out/); + assert.ok(!detail.includes("Retry review"), "No retry for a succeeded review"); + review.status = "failed"; + review.error = "agent timed out after 900s"; + const failedDetail = await (await get(`/reviews/${reviewId}`)).text(); + assert.match(failedDetail, /The agent took too long/); + assert.match(failedDetail, /Error details/); + assert.match(failedDetail, /Retry review/); + console.log("Submitting retry action"); + const retry = await fetch(origin + `/reviews/${reviewId}`, { + method: "POST", + headers: { Cookie: sessionCookie, Origin: origin }, + body: actionForm(failedDetail, "Retry review"), + redirect: "manual", + signal: AbortSignal.timeout(15_000), + }); + assert.equal(retry.headers.get("location"), `/reviews/${reviewId}?done=retry`); + assert.deepEqual(controls, [`/api/v1/reviews/${reviewId}/retry`]); + review.status = "succeeded"; + review.error = null; assert.match(await (await get("/reviews/invalid")).text(), /Page not found/); empty = true; assert.match( @@ -643,7 +733,7 @@ try { `Sign-out clears ${name}`, ); console.log( - "PASS: standalone runtime; public docs, Better Auth GitHub flow, landing, installations, overview, open pull requests and GitHub failure, vulnerabilities, fix priority queue and repository pages, settings and models, admin guard, pagination, review escaping, empty/404/401, expired token, disabled token endpoints, sign-in and sign-out actions.", + "PASS: standalone runtime; public docs, agent health, review retry, digest settings, Better Auth GitHub flow, landing, installations, overview, open pull requests and GitHub failure, vulnerabilities, fix priority queue and repository pages, settings and models, admin guard, pagination, review escaping, empty/404/401, expired token, disabled token endpoints, sign-in and sign-out actions.", ); } catch (error) { console.error(output, calls); From 1d529f70e8ec05a8d223cf4413fb1dedefc5f888 Mon Sep 17 00:00:00 2001 From: Gaya KACI <163902005+kacigaya@users.noreply.github.com> Date: Tue, 3 Nov 2026 08:19:37 +0000 Subject: [PATCH 2/2] feat(digest): encrypt webhook URL at rest Seal the URL with AES-256-GCM under an HKDF key derived from the GitHub App private key. Bind the installation ID as associated data. Keep only the host in plain text for display. A database dump alone no longer reveals the URL. After an App key rotation the digest reports an error and the admin enters the URL again. --- Cargo.lock | 1 + crates/server/Cargo.toml | 1 + crates/server/src/api.rs | 22 +++-- crates/server/src/db.rs | 38 ++++---- crates/server/src/digest.rs | 118 ++++++++++++++++++++-- crates/server/src/main.rs | 2 + crates/server/src/tests.rs | 124 +++++++++++++++--------- docs/configuration.md | 3 + docs/security.md | 8 +- migrations/0004_usage_health_digest.sql | 8 +- web/components/settings-form.tsx | 2 +- web/lib/docs-content.tsx | 2 +- 12 files changed, 247 insertions(+), 82 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0fe7234..269be63 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -421,6 +421,7 @@ dependencies = [ "hex", "hmac 0.13.0", "octocrab", + "ring", "serde", "serde_json", "sha2 0.11.1", diff --git a/crates/server/Cargo.toml b/crates/server/Cargo.toml index 4cdfff1..dd0fa81 100644 --- a/crates/server/Cargo.toml +++ b/crates/server/Cargo.toml @@ -20,6 +20,7 @@ crocode-core.workspace = true hex.workspace = true hmac = "0.13" octocrab = "0.54" +ring = "0.17" serde.workspace = true serde_json.workspace = true sha2.workspace = true diff --git a/crates/server/src/api.rs b/crates/server/src/api.rs index 5cce99b..e0e9da7 100644 --- a/crates/server/src/api.rs +++ b/crates/server/src/api.rs @@ -280,11 +280,8 @@ fn view(i: db::Installation, can_admin: bool) -> InstallationView { model: i.model, }, digest: DigestView { - enabled: i.digest_webhook_url.is_some(), - webhook_host: i - .digest_webhook_url - .as_deref() - .and_then(crate::digest::webhook_host), + enabled: i.digest_webhook_sealed.is_some(), + webhook_host: i.digest_webhook_host, sent_at: i.digest_sent_at, attempted_at: i.digest_attempted_at, error: i.digest_error, @@ -388,14 +385,25 @@ async fn update_settings( Some(None) => None, None => current.model, }; - let digest = match &patch.digest_webhook_url { + let sealed = match &patch.digest_webhook_url { Some(Some(url)) => { crate::digest::validate_webhook_url(url).map_err(unprocessable)?; - Some(Some(url.as_str())) + let host = crate::digest::webhook_host(url) + .ok_or_else(|| unprocessable("invalid webhook URL"))?; + let sealed = state.webhook_key.seal(id, url).map_err(|err| { + tracing::error!(error = err, "seal webhook URL"); + ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, "internal error") + })?; + Some(Some((sealed, host))) } Some(None) => Some(None), None => None, }; + let digest = sealed.as_ref().map(|value| { + value + .as_ref() + .map(|(sealed, host)| (sealed.as_str(), host.as_str())) + }); let enabled = patch.enabled.unwrap_or(current.enabled); db::update_settings( &state.db, diff --git a/crates/server/src/db.rs b/crates/server/src/db.rs index c106d28..358ba49 100644 --- a/crates/server/src/db.rs +++ b/crates/server/src/db.rs @@ -61,8 +61,9 @@ pub struct Installation { pub agent: Option, pub model: Option, pub created_at: DateTime, - /// Secret. Never return it from the API. - pub digest_webhook_url: Option, + /// The webhook URL, sealed. Open it only to send. See `digest::WebhookKey`. + pub digest_webhook_sealed: Option, + pub digest_webhook_host: Option, pub digest_sent_at: Option>, pub digest_attempted_at: Option>, pub digest_error: Option, @@ -71,7 +72,7 @@ pub struct Installation { const INSTALLATION_COLUMNS: &str = "id, account_id, account_login, account_type, repository_selection, suspended, enabled, agent, model, created_at, - digest_webhook_url, digest_sent_at, digest_attempted_at, digest_error"; + digest_webhook_sealed, digest_webhook_host, digest_sent_at, digest_attempted_at, digest_error"; pub struct InstallationUpsert<'a> { pub id: i64, @@ -148,24 +149,25 @@ pub async fn installations(db: &Db, ids: &[i64]) -> sqlx::Result, model: Option<&str>, - digest_webhook_url: Option>, + digest: Option>, ) -> sqlx::Result<()> { sqlx::query( "UPDATE installations SET enabled = $2, agent = $3, model = $4, - digest_webhook_url = CASE WHEN $5 THEN $6 ELSE digest_webhook_url END, - digest_attempted_at = CASE WHEN $5 AND $6 IS DISTINCT FROM digest_webhook_url + digest_webhook_sealed = CASE WHEN $5 THEN $6 ELSE digest_webhook_sealed END, + digest_webhook_host = CASE WHEN $5 THEN $7 ELSE digest_webhook_host END, + digest_attempted_at = CASE WHEN $5 AND $6 IS DISTINCT FROM digest_webhook_sealed THEN NULL ELSE digest_attempted_at END, - digest_error = CASE WHEN $5 AND $6 IS DISTINCT FROM digest_webhook_url + digest_error = CASE WHEN $5 AND $6 IS DISTINCT FROM digest_webhook_sealed THEN NULL ELSE digest_error END, - digest_sent_at = CASE WHEN $5 AND $6 IS DISTINCT FROM digest_webhook_url + digest_sent_at = CASE WHEN $5 AND $6 IS DISTINCT FROM digest_webhook_sealed THEN NULL ELSE digest_sent_at END, updated_at = now() WHERE id = $1", @@ -174,8 +176,9 @@ pub async fn update_settings( .bind(enabled) .bind(agent) .bind(model) - .bind(digest_webhook_url.is_some()) - .bind(digest_webhook_url.flatten()) + .bind(digest.is_some()) + .bind(digest.flatten().map(|(sealed, _)| sealed)) + .bind(digest.flatten().map(|(_, host)| host)) .execute(db) .await?; Ok(()) @@ -998,7 +1001,8 @@ pub async fn review_contents( pub struct DigestJob { pub id: i64, pub account_login: String, - pub digest_webhook_url: String, + /// Sealed. See `digest::WebhookKey`. + pub digest_webhook_sealed: String, pub digest_sent_at: Option>, } @@ -1009,13 +1013,13 @@ pub async fn claim_digest(db: &Db) -> sqlx::Result> { "UPDATE installations SET digest_attempted_at = now() WHERE id = ( SELECT id FROM installations - WHERE digest_webhook_url IS NOT NULL AND NOT deleted AND NOT suspended + WHERE digest_webhook_sealed IS NOT NULL AND NOT deleted AND NOT suspended AND (digest_sent_at IS NULL OR digest_sent_at <= now() - interval '7 days') AND (digest_attempted_at IS NULL OR digest_attempted_at <= now() - interval '1 hour') ORDER BY digest_attempted_at NULLS FIRST FOR UPDATE SKIP LOCKED LIMIT 1) - RETURNING id, account_login, digest_webhook_url, digest_sent_at", + RETURNING id, account_login, digest_webhook_sealed, digest_sent_at", ) .fetch_optional(db) .await @@ -1032,10 +1036,10 @@ pub async fn finish_digest( sqlx::query( "UPDATE installations SET digest_error = $3, digest_sent_at = COALESCE($4, digest_sent_at) - WHERE id = $1 AND digest_webhook_url = $2", + WHERE id = $1 AND digest_webhook_sealed = $2", ) .bind(job.id) - .bind(&job.digest_webhook_url) + .bind(&job.digest_webhook_sealed) .bind(result.err()) .bind(result.ok()) .execute(db) diff --git a/crates/server/src/digest.rs b/crates/server/src/digest.rs index 79ea7ad..c4398c8 100644 --- a/crates/server/src/digest.rs +++ b/crates/server/src/digest.rs @@ -1,6 +1,7 @@ //! Opt-in weekly security digest. //! -//! An installation admin sets an HTTPS webhook URL. Every 7 days the worker +//! An installation admin sets an HTTPS webhook URL. The database holds it +//! sealed with AES-256-GCM, see [`WebhookKey`]. Every 7 days the worker //! reads the Dependabot alerts of the installation with a read-only token and //! posts a Slack-compatible message (`{"text": ...}`) to that URL. //! @@ -16,8 +17,12 @@ use std::time::Duration; use anyhow::{Context, Result}; use axum::http::Uri; +use base64::Engine; +use base64::engine::general_purpose::STANDARD; use chrono::{DateTime, Utc}; use octocrab::{Octocrab, Page}; +use ring::rand::{SecureRandom, SystemRandom}; +use ring::{aead, hkdf}; use serde::Deserialize; use serde::de::DeserializeOwned; use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt}; @@ -45,6 +50,71 @@ const DELIVER_TIMEOUT: Duration = Duration::from_secs(45 * 60); /// Items listed per section. Counts always cover every alert. const LIST_ITEMS: usize = 5; +/// Seals webhook URLs at rest. The AES-256-GCM key comes from the GitHub App +/// private key through HKDF-SHA256, so a database dump or backup alone does +/// not reveal a URL. The installation ID is the associated data: a sealed +/// value copied to another installation does not open. Rotating the App key +/// makes stored URLs unreadable; the admin enters the URL again. +pub struct WebhookKey(aead::LessSafeKey); + +const SEALED_PREFIX: &str = "v1:"; + +impl WebhookKey { + pub fn derive(app_private_key: &[u8]) -> Self { + // The API and worker may read the same PEM with different line + // endings or trailing whitespace. + let normalized: Vec = String::from_utf8_lossy(app_private_key) + .replace("\r\n", "\n") + .trim() + .as_bytes() + .to_vec(); + let prk = hkdf::Salt::new(hkdf::HKDF_SHA256, b"crocode").extract(&normalized); + let okm = prk + .expand(&[b"digest webhook url v1"], &aead::AES_256_GCM) + .expect("AES-256 key length is a valid HKDF output length"); + Self(aead::LessSafeKey::new(aead::UnboundKey::from(okm))) + } + + pub fn seal(&self, installation: i64, url: &str) -> Result { + let mut nonce = [0u8; aead::NONCE_LEN]; + SystemRandom::new() + .fill(&mut nonce) + .map_err(|_| "random number generator failed")?; + let mut data = url.as_bytes().to_vec(); + self.0 + .seal_in_place_append_tag( + aead::Nonce::assume_unique_for_key(nonce), + aead::Aad::from(installation.to_be_bytes()), + &mut data, + ) + .map_err(|_| "cannot seal the webhook URL")?; + Ok(format!( + "{SEALED_PREFIX}{}", + STANDARD.encode([nonce.as_slice(), &data].concat()) + )) + } + + /// `None` when the value is damaged, belongs to another installation, or + /// was sealed with another key. + pub fn open(&self, installation: i64, sealed: &str) -> Option { + let mut bytes = STANDARD.decode(sealed.strip_prefix(SEALED_PREFIX)?).ok()?; + if bytes.len() < aead::NONCE_LEN { + return None; + } + let mut data = bytes.split_off(aead::NONCE_LEN); + let nonce = aead::Nonce::try_assume_unique_for_key(&bytes).ok()?; + let plain = self + .0 + .open_in_place( + nonce, + aead::Aad::from(installation.to_be_bytes()), + &mut data, + ) + .ok()?; + String::from_utf8(plain.to_vec()).ok() + } +} + /// Days an open alert may stay open before the digest calls it overdue. pub fn deadline_days(severity: &str) -> Option { match severity { @@ -541,6 +611,13 @@ pub async fn send(url: &str, text: &str) -> Result<(), String> { /// Returns the start of the alert read on success. async fn deliver(state: &AppState, job: &db::DigestJob) -> Result, String> { + let url = state + .webhook_key + .open(job.id, &job.digest_webhook_sealed) + .ok_or_else(|| { + "cannot decrypt the webhook URL; the GitHub App key may have changed, so enter the URL again" + .to_owned() + })?; let now = Utc::now(); let since = job .digest_sent_at @@ -575,15 +652,12 @@ async fn deliver(state: &AppState, job: &db::DigestJob) -> Result, match db::installation(&state.db, job.id).await { Ok(Some(i)) if !i.suspended - && i.digest_webhook_url.as_deref() == Some(job.digest_webhook_url.as_str()) => {} + && i.digest_webhook_sealed.as_deref() + == Some(job.digest_webhook_sealed.as_str()) => {} Ok(_) => return Err("digest settings changed before sending".into()), Err(_) => return Err("database error before sending".into()), } - send( - &job.digest_webhook_url, - &render(&job.account_login, &digest), - ) - .await?; + send(&url, &render(&job.account_login, &digest)).await?; Ok(now) } @@ -632,6 +706,36 @@ pub async fn run(state: AppState, shutdown: CancellationToken) { #[cfg(test)] mod tests { use super::*; + + #[test] + fn webhook_urls_are_sealed_per_installation_and_key() { + let key = WebhookKey::derive(b"-----BEGIN KEY-----\nabc\n-----END KEY-----\n"); + let url = "https://hooks.slack.com/services/T0/B0/secret"; + let sealed = key.seal(7, url).unwrap(); + assert!(sealed.starts_with("v1:") && !sealed.contains("secret")); + assert_ne!( + sealed, + key.seal(7, url).unwrap(), + "each seal uses a new nonce" + ); + assert_eq!(key.open(7, &sealed).as_deref(), Some(url)); + // The same PEM with other line endings derives the same key. + let crlf = WebhookKey::derive(b"-----BEGIN KEY-----\r\nabc\r\n-----END KEY-----"); + assert_eq!(crlf.open(7, &sealed).as_deref(), Some(url)); + assert_eq!(key.open(8, &sealed), None, "another installation"); + assert_eq!( + WebhookKey::derive(b"other").open(7, &sealed), + None, + "another key" + ); + let mut tampered = sealed.clone().into_bytes(); + let last = tampered.len() - 2; + tampered[last] = if tampered[last] == b'A' { b'B' } else { b'A' }; + assert_eq!(key.open(7, &String::from_utf8(tampered).unwrap()), None); + for bad in ["", "v1:", "v1:AAAA", url] { + assert_eq!(key.open(7, bad), None, "{bad}"); + } + } use axum::extract::OriginalUri; use axum::response::IntoResponse; use axum::{Router, routing::get}; diff --git a/crates/server/src/main.rs b/crates/server/src/main.rs index c605bf5..944013f 100644 --- a/crates/server/src/main.rs +++ b/crates/server/src/main.rs @@ -36,6 +36,7 @@ pub struct AppState { pub auth: Arc, pub pulls: Arc, pub vulnerability_checks: Arc, + pub webhook_key: Arc, } const USAGE: &str = "usage: crocode "; @@ -75,6 +76,7 @@ async fn main() -> Result<()> { &settings.github_private_key, &settings.github_api_url, )?, + webhook_key: Arc::new(digest::WebhookKey::derive(&settings.github_private_key)), settings, auth: Arc::default(), pulls: Arc::default(), diff --git a/crates/server/src/tests.rs b/crates/server/src/tests.rs index 388c951..532e0c1 100644 --- a/crates/server/src/tests.rs +++ b/crates/server/src/tests.rs @@ -23,6 +23,7 @@ fn state(pool: PgPool) -> AppState { auth: Arc::default(), pulls: Arc::default(), vulnerability_checks: Arc::default(), + webhook_key: Arc::new(crate::digest::WebhookKey::derive(b"test-key")), } } @@ -1141,22 +1142,69 @@ async fn usage_is_stored_and_grouped_by_agent(pool: PgPool) { #[sqlx::test(migrations = "../../migrations")] #[ignore = "needs DATABASE_URL"] -async fn digest_opt_in_claims_once_and_hides_the_url(pool: PgPool) { +async fn digest_opt_in_claims_once_and_seals_the_url(pool: PgPool) { installation(&pool, 1).await; installation(&pool, 2).await; assert!(db::claim_digest(&pool).await.unwrap().is_none()); + let state = state(pool.clone()); + with_access(&state, "admin", &[(1, true), (2, true)]); + let patch = |id: i64, body: Value| { + let state = state.clone(); + async move { + router(state) + .oneshot( + Request::patch(format!("/api/v1/installations/{id}/settings")) + .header("authorization", "Bearer admin") + .header("content-type", "application/json") + .body(Body::from(body.to_string())) + .unwrap(), + ) + .await + .unwrap() + .status() + } + }; + for bad in [ + "http://hooks.example.com/x", + "https://10.0.0.1/x", + "https://localhost/x", + ] { + assert_eq!( + patch(1, json!({ "digest_webhook_url": bad })).await, + StatusCode::UNPROCESSABLE_ENTITY, + "{bad}" + ); + } let url = "https://hooks.example.com/services/secret-path"; - db::update_settings(&pool, 1, true, None, None, Some(Some(url))) - .await - .unwrap(); + assert_eq!( + patch(1, json!({ "digest_webhook_url": url })).await, + StatusCode::OK + ); + + // The database holds only the sealed value and the host. + let row: String = + sqlx::query_scalar("SELECT row_to_json(i)::text FROM installations i WHERE id = 1") + .fetch_one(&pool) + .await + .unwrap(); + assert!(!row.contains("secret-path"), "{row}"); let job = db::claim_digest(&pool).await.unwrap().unwrap(); - assert_eq!((job.id, job.digest_webhook_url.as_str()), (1, url)); + assert_eq!(job.id, 1); + assert_eq!( + state + .webhook_key + .open(1, &job.digest_webhook_sealed) + .as_deref(), + Some(url) + ); + assert_eq!(state.webhook_key.open(2, &job.digest_webhook_sealed), None); assert!(db::claim_digest(&pool).await.unwrap().is_none()); db::finish_digest(&pool, &job, Err("webhook returned HTTP 404")) .await .unwrap(); let i = db::installation(&pool, 1).await.unwrap().unwrap(); assert_eq!(i.digest_error.as_deref(), Some("webhook returned HTTP 404")); + assert_eq!(i.digest_webhook_host.as_deref(), Some("hooks.example.com")); assert!(i.digest_sent_at.is_none()); // A failed send waits an hour before the next attempt. @@ -1181,55 +1229,43 @@ async fn digest_opt_in_claims_once_and_hides_the_url(pool: PgPool) { .unwrap(); assert!(db::claim_digest(&pool).await.unwrap().is_none()); - // A result for an old URL is dropped after the admin changes it. - let other = "https://hooks.example.com/services/other"; - db::update_settings(&pool, 1, true, None, None, Some(Some(other))) - .await - .unwrap(); + // A result for an old URL is dropped after the admin changes it, and the + // new URL gets its first digest at once. + assert_eq!( + patch( + 1, + json!({ "digest_webhook_url": "https://chat.example.org/hooks/other" }) + ) + .await, + StatusCode::OK + ); db::finish_digest(&pool, &job, Err("old")).await.unwrap(); let i = db::installation(&pool, 1).await.unwrap().unwrap(); assert!(i.digest_error.is_none() && i.digest_attempted_at.is_none()); - // A new URL gets its first digest at once. assert!(i.digest_sent_at.is_none()); + assert_eq!(i.digest_webhook_host.as_deref(), Some("chat.example.org")); assert_eq!(db::claim_digest(&pool).await.unwrap().unwrap().id, 1); - let state = state(pool.clone()); - with_access(&state, "admin", &[(1, true)]); - for bad in [ - "http://hooks.example.com/x", - "https://10.0.0.1/x", - "https://localhost/x", - ] { - let response = router(state.clone()) - .oneshot( - Request::patch("/api/v1/installations/1/settings") - .header("authorization", "Bearer admin") - .header("content-type", "application/json") - .body(Body::from(json!({ "digest_webhook_url": bad }).to_string())) - .unwrap(), - ) - .await - .unwrap(); - assert_eq!(response.status(), StatusCode::UNPROCESSABLE_ENTITY, "{bad}"); - } let (status, body) = call(&state, "GET", "/api/v1/installations/1", "admin").await; assert_eq!(status, StatusCode::OK); assert_eq!(body["digest"]["enabled"], true); - assert_eq!(body["digest"]["webhook_host"], "hooks.example.com"); - assert!(!body.to_string().contains("services/")); + assert_eq!(body["digest"]["webhook_host"], "chat.example.org"); + assert!(!body.to_string().contains("hooks/other")); + assert!(!body.to_string().contains("v1:")); - db::update_settings(&pool, 1, true, None, None, Some(None)) - .await - .unwrap(); + assert_eq!( + patch(1, json!({ "digest_webhook_url": null })).await, + StatusCode::OK + ); let i = db::installation(&pool, 1).await.unwrap().unwrap(); - assert!(i.digest_webhook_url.is_none() && i.digest_sent_at.is_none()); - // `None` leaves the digest unchanged. - db::update_settings(&pool, 2, true, None, None, Some(Some(url))) - .await - .unwrap(); - db::update_settings(&pool, 2, false, None, None, None) - .await - .unwrap(); + assert!(i.digest_webhook_sealed.is_none() && i.digest_webhook_host.is_none()); + assert!(i.digest_sent_at.is_none()); + // Leaving the field out keeps the digest. + assert_eq!( + patch(2, json!({ "digest_webhook_url": url })).await, + StatusCode::OK + ); + assert_eq!(patch(2, json!({ "enabled": false })).await, StatusCode::OK); let i = db::installation(&pool, 2).await.unwrap().unwrap(); - assert_eq!(i.digest_webhook_url.as_deref(), Some(url)); + assert!(i.digest_webhook_sealed.is_some()); } diff --git a/docs/configuration.md b/docs/configuration.md index 869408d..79778df 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -185,6 +185,9 @@ same window, so no alert change is lost. Dismissed alerts are not counted. This is GitHub's alert inventory, not a full security scan. The worker checks for due digests every 10 minutes. After a failed send it tries again every hour. +The database stores the webhook URL encrypted with a key derived from the GitHub App private key. +After you rotate that private key, the worker cannot read stored URLs. The settings page then shows +an error. Enter the webhook URL again to fix it. The settings page shows the last send and the last error. The GitHub App needs the read-only **Dependabot alerts** permission. See [Enable the vulnerability panel](github-app.md#enable-the-vulnerability-panel-on-an-existing-app). diff --git a/docs/security.md b/docs/security.md index 028a460..4d2113a 100644 --- a/docs/security.md +++ b/docs/security.md @@ -183,8 +183,12 @@ Model IDs that contain a known secret value are dropped. Codex list runs receive The weekly security digest is opt-in. It sends repository names, package names, and advisory IDs from the worker to the webhook host that an admin chose. That host and its chat service receive this data. -The webhook URL works like a password. The database stores it in plain text. -The API never returns it, and error messages never contain it. +The webhook URL works like a password. The database stores it sealed with AES-256-GCM. +The key comes from the GitHub App private key through HKDF-SHA256. The database never holds that key, +so a database dump or backup alone does not reveal the URL. The installation ID is bound to each sealed value. +A host that holds both the database and the App private key can still read the URL. +The database keeps the webhook host in plain text for display. +The API never returns the URL, and error messages never contain it. The worker sends one HTTPS request to port 443 and follows no redirects. It resolves the host once and refuses the send when any address is private, loopback, link-local, shared, reserved, or otherwise not public. It then connects to a checked address. diff --git a/migrations/0004_usage_health_digest.sql b/migrations/0004_usage_health_digest.sql index 612db77..6f26c0f 100644 --- a/migrations/0004_usage_health_digest.sql +++ b/migrations/0004_usage_health_digest.sql @@ -15,10 +15,12 @@ ALTER TABLE agent_models ALTER COLUMN checked_at SET NOT NULL, ALTER COLUMN checked_at SET DEFAULT now(); --- Opt-in weekly security digest. The webhook URL is a secret: the API never --- returns it. +-- Opt-in weekly security digest. The webhook URL is a secret. It is stored +-- sealed with AES-256-GCM under a key derived from the GitHub App private +-- key, which the database never holds. The host is kept for display. ALTER TABLE installations - ADD COLUMN digest_webhook_url TEXT, + ADD COLUMN digest_webhook_sealed TEXT, + ADD COLUMN digest_webhook_host TEXT, ADD COLUMN digest_sent_at TIMESTAMPTZ, ADD COLUMN digest_attempted_at TIMESTAMPTZ, ADD COLUMN digest_error TEXT; diff --git a/web/components/settings-form.tsx b/web/components/settings-form.tsx index 568baf9..c4787cf 100644 --- a/web/components/settings-form.tsx +++ b/web/components/settings-form.tsx @@ -265,7 +265,7 @@ export function SettingsForm({ : ""} Use an HTTPS Slack or Mattermost incoming webhook, or a Discord webhook URL that ends in /slack. CroCode stores the URL - as a secret and never shows it again. + encrypted and never shows it again.

    )} diff --git a/web/lib/docs-content.tsx b/web/lib/docs-content.tsx index 1a2468b..6b8e98d 100644 --- a/web/lib/docs-content.tsx +++ b/web/lib/docs-content.tsx @@ -277,7 +277,7 @@ export const DOCS_CONTENT: Record = { [critical, "15 days"], [high, "30 days"], [medium, "90 days"], [low, "180 days"], ]} /> -

    The message names repositories, packages, and CVE or GHSA IDs. It leaves GitHub. The API never returns the webhook URL. The worker sends only to public addresses on port 443 and follows no redirects. After a failed send it tries again every hour.

    +

    The message names repositories, packages, and CVE or GHSA IDs. It leaves GitHub. The database stores the webhook URL encrypted with a key derived from the GitHub App private key, and the API never returns it. After a private key rotation, enter the URL again. The worker sends only to public addresses on port 443 and follows no redirects. After a failed send it tries again every hour.

    , }, ],