From d892e775740a77c9ffcdefd216403ec530c43853 Mon Sep 17 00:00:00 2001
From: Jonathan Haas <jonathan@haas.holdings>
Date: Wed, 11 Mar 2026 12:43:08 -0700
Subject: [PATCH 1/2] =?UTF-8?q?feat:=209=20quality=20improvements=20?=
 =?UTF-8?q?=E2=80=94=20multi-model=20routing,=20incremental=20reviews,=20s?=
 =?UTF-8?q?tructured=20logging,=20tests?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- Fix panic-prone unwrap() calls in production paths (adapters, verification)
- Add 82 new tests: API layer (36), database layer (27), state (2), config (4), incremental SHA (2), and more
- GitHub suggestion blocks: multi-line support with start_line/line for one-click PR fixes
- Incremental review: push-by-push delta using last_reviewed_shas and compare API
- Multi-model routing: ModelRole::Fast cascades model_fast → model_weak → primary
- Wire up dead code: InteractiveCommand → webhook, storage → API, model_name → pipeline, detect_context_window → doctor
- Remove unused Plugin trait, legacy Ollama types
- Replace all eprintln!/println! with tracing::error!/warn!/info! in server modules
- Add delete_review and prune_reviews API endpoints
- Reduce cloning by inlining OllamaShowRequest/Response

Closes #8, #16, #26

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---
 src/adapters/llm.rs          |   6 +-
 src/adapters/ollama.rs       |  73 +---
 src/commands/doctor.rs       |  48 +--
 src/commands/git.rs          |   6 +-
 src/commands/pr.rs           |   6 +-
 src/commands/smart_review.rs |  18 +-
 src/config.rs                |  58 +++
 src/core/interactive.rs      |  10 +-
 src/core/offline.rs          |   2 -
 src/plugins/plugin.rs        |  15 -
 src/review/pipeline.rs       |   1 +
 src/review/verification.rs   |  28 +-
 src/server/api.rs            | 674 ++++++++++++++++++++++++++++++++++-
 src/server/github.rs         | 247 +++++++++++--
 src/server/mod.rs            |   9 +-
 src/server/state.rs          | 148 +++++++-
 src/server/storage.rs        |   1 -
 src/server/storage_json.rs   |  15 +-
 src/server/storage_pg.rs     | 181 ++++++++++
 19 files changed, 1333 insertions(+), 213 deletions(-)

diff --git a/src/adapters/llm.rs b/src/adapters/llm.rs
index f1ae106..2311259 100644
--- a/src/adapters/llm.rs
+++ b/src/adapters/llm.rs
@@ -65,7 +65,6 @@ pub struct Usage {
 #[async_trait]
 pub trait LLMAdapter: Send + Sync {
     async fn complete(&self, request: LLMRequest) -> Result<LLMResponse>;
-    #[allow(dead_code)]
     fn model_name(&self) -> &str;
 }
 
@@ -99,9 +98,8 @@ pub fn create_adapter(config: &ModelConfig) -> Result<Box<dyn LLMAdapter>> {
     }
 
     // Vendor-prefixed model IDs (vendor/model)
-    if config.model_name.contains('/') {
-        let (vendor, model) = config.model_name.split_once('/').unwrap();
-        let model = model.to_string();
+    if let Some((vendor, model_suffix)) = config.model_name.split_once('/') {
+        let model = model_suffix.to_string();
         match vendor {
             "anthropic" => {
                 // Route anthropic/ prefix directly to the Anthropic adapter
diff --git a/src/adapters/ollama.rs b/src/adapters/ollama.rs
index 40683cd..3feff5a 100644
--- a/src/adapters/ollama.rs
+++ b/src/adapters/ollama.rs
@@ -43,57 +43,7 @@ struct OllamaChatResponse {
     eval_count: Option<usize>,
 }
 
-// -- Legacy generate API types (kept for reference / future use) --
-
-#[allow(dead_code)]
-#[derive(Serialize)]
-struct OllamaGenerateRequest {
-    model: String,
-    prompt: String,
-    system: String,
-    temperature: f32,
-    num_predict: usize,
-    stream: bool,
-}
-
-#[allow(dead_code)]
-#[derive(Deserialize)]
-struct OllamaGenerateResponse {
-    response: String,
-    model: String,
-    done: bool,
-    _context: Option<Vec<i32>>,
-    _total_duration: Option<u64>,
-    prompt_eval_count: Option<usize>,
-    eval_count: Option<usize>,
-}
-
 // -- /api/show types for context window detection --
-
-#[allow(dead_code)]
-#[derive(Serialize)]
-struct OllamaShowRequest {
-    name: String,
-}
-
-#[allow(dead_code)]
-#[derive(Deserialize)]
-struct OllamaShowResponse {
-    #[serde(default)]
-    parameters: Option<String>,
-    #[allow(dead_code)]
-    #[serde(default)]
-    details: Option<OllamaShowDetails>,
-}
-
-#[allow(dead_code)]
-#[derive(Deserialize)]
-struct OllamaShowDetails {
-    family: Option<String>,
-    parameter_size: Option<String>,
-    quantization_level: Option<String>,
-}
-
 impl OllamaAdapter {
     pub fn new(config: ModelConfig) -> Result<Self> {
         let base_url = config
@@ -131,27 +81,20 @@ impl OllamaAdapter {
     ///
     /// Returns `Some(num_ctx)` if the model metadata contains a `num_ctx` parameter,
     /// or `None` if the endpoint is unreachable or the parameter is not found.
-    #[allow(dead_code)]
+    #[allow(dead_code)] // Public API, used in tests; production uses OfflineModelManager
     pub async fn detect_context_window(&self) -> Option<usize> {
         let url = format!("{}/api/show", self.base_url);
-        let show_request = OllamaShowRequest {
-            name: self.model_name_bare().to_string(),
-        };
+        let body = serde_json::json!({"name": self.model_name_bare()});
 
-        let response = self
-            .client
-            .post(&url)
-            .json(&show_request)
-            .send()
-            .await
-            .ok()?;
+        let response = self.client.post(&url).json(&body).send().await.ok()?;
 
         if !response.status().is_success() {
             return None;
         }
 
-        let show_response: OllamaShowResponse = response.json().await.ok()?;
-        parse_num_ctx(show_response.parameters.as_deref())
+        let value: serde_json::Value = response.json().await.ok()?;
+        let parameters = value.get("parameters").and_then(|p| p.as_str());
+        parse_num_ctx(parameters)
     }
 }
 
@@ -162,8 +105,8 @@ impl OllamaAdapter {
 /// num_ctx 4096
 /// temperature 0.8
 /// ```
-#[allow(dead_code)]
-fn parse_num_ctx(parameters: Option<&str>) -> Option<usize> {
+#[allow(dead_code)] // Called by detect_context_window
+pub(crate) fn parse_num_ctx(parameters: Option<&str>) -> Option<usize> {
     let params = parameters?;
     for line in params.lines() {
         let line = line.trim();
diff --git a/src/commands/doctor.rs b/src/commands/doctor.rs
index e625cfc..152d83a 100644
--- a/src/commands/doctor.rs
+++ b/src/commands/doctor.rs
@@ -131,9 +131,7 @@ pub async fn doctor_command(config: Config) -> Result<()> {
 
             // Context window detection (Ollama only)
             if endpoint_type == "ollama" {
-                if let Some(ctx_size) =
-                    detect_model_context_window(&client, &base_url, &recommended.name).await
-                {
+                if let Ok(Some(ctx_size)) = manager.detect_context_window(&recommended.name).await {
                     println!(
                         "  Context window: {} tokens (detected from model)",
                         ctx_size
@@ -289,50 +287,6 @@ fn estimate_tokens(text: &str) -> usize {
     (text.len() / 4).max(1)
 }
 
-/// Query Ollama's `/api/show` to detect the model's context window size.
-async fn detect_model_context_window(
-    client: &Client,
-    base_url: &str,
-    model_name: &str,
-) -> Option<usize> {
-    let url = format!("{}/api/show", base_url);
-    let body = serde_json::json!({"name": model_name});
-    let resp = client.post(&url).json(&body).send().await.ok()?;
-    if !resp.status().is_success() {
-        return None;
-    }
-    let text = resp.text().await.ok()?;
-    let value: serde_json::Value = serde_json::from_str(&text).ok()?;
-
-    // The "parameters" field is a newline-delimited string of key-value pairs
-    if let Some(params) = value.get("parameters").and_then(|p| p.as_str()) {
-        for line in params.lines() {
-            let trimmed = line.trim();
-            if trimmed.starts_with("num_ctx") {
-                if let Some(val) = trimmed.split_whitespace().nth(1) {
-                    return val.parse().ok();
-                }
-            }
-        }
-    }
-
-    // Also check model_info for context_length
-    if let Some(info) = value.get("model_info") {
-        // Try common key patterns
-        for key in &[
-            "context_length",
-            "llama.context_length",
-            "general.context_length",
-        ] {
-            if let Some(ctx) = info.get(*key).and_then(|v| v.as_u64()) {
-                return Some(ctx as usize);
-            }
-        }
-    }
-
-    None
-}
-
 /// Print system resource information (RAM and GPU if available).
 fn check_system_resources() {
     println!("System Resources:");
diff --git a/src/commands/git.rs b/src/commands/git.rs
index 2bb467b..adffcce 100644
--- a/src/commands/git.rs
+++ b/src/commands/git.rs
@@ -72,7 +72,8 @@ async fn suggest_commit_message(config: config::Config) -> Result<()> {
         return Ok(());
     }
 
-    let model_config = config.to_model_config();
+    // Use Fast model for commit message suggestion (lightweight task)
+    let model_config = config.to_model_config_for_role(config::ModelRole::Fast);
 
     let adapter = adapters::llm::create_adapter(&model_config)?;
 
@@ -114,7 +115,8 @@ async fn suggest_pr_title(config: config::Config) -> Result<()> {
         return Ok(());
     }
 
-    let model_config = config.to_model_config();
+    // Use Fast model for PR title suggestion (lightweight task)
+    let model_config = config.to_model_config_for_role(config::ModelRole::Fast);
 
     let adapter = adapters::llm::create_adapter(&model_config)?;
 
diff --git a/src/commands/pr.rs b/src/commands/pr.rs
index 246ce26..296b793 100644
--- a/src/commands/pr.rs
+++ b/src/commands/pr.rs
@@ -81,9 +81,9 @@ pub async fn pr_command(
         let diffs = core::DiffParser::parse_unified_diff(&diff_content)?;
         let git = core::GitIntegration::new(".")?;
 
-        let model_config = config.to_model_config();
-
-        let adapter = adapters::llm::create_adapter(&model_config)?;
+        // Use Fast model for PR summary generation (lightweight task)
+        let fast_config = config.to_model_config_for_role(config::ModelRole::Fast);
+        let adapter = adapters::llm::create_adapter(&fast_config)?;
         let options = core::SummaryOptions {
             include_diagram: config.smart_review_diagram,
         };
diff --git a/src/commands/smart_review.rs b/src/commands/smart_review.rs
index ea22433..4fc4cf7 100644
--- a/src/commands/smart_review.rs
+++ b/src/commands/smart_review.rs
@@ -62,6 +62,20 @@ pub async fn smart_review_command(
     let model_config = config.to_model_config();
 
     let adapter = adapters::llm::create_adapter(&model_config)?;
+
+    // Use Fast model for PR summary and diagram generation (lightweight tasks)
+    let fast_config = config.to_model_config_for_role(config::ModelRole::Fast);
+    let fast_adapter: Box<dyn adapters::llm::LLMAdapter> =
+        if fast_config.model_name != model_config.model_name {
+            info!(
+                "Using fast model '{}' for PR summary/diagram",
+                fast_config.model_name
+            );
+            adapters::llm::create_adapter(&fast_config)?
+        } else {
+            adapters::llm::create_adapter(&model_config)?
+        };
+
     let mut all_comments = Vec::new();
     let mut pr_summary = if config.smart_review_summary {
         match core::GitIntegration::new(&repo_root) {
@@ -72,7 +86,7 @@ pub async fn smart_review_command(
                 match core::PRSummaryGenerator::generate_summary_with_options(
                     &diffs,
                     &git,
-                    adapter.as_ref(),
+                    fast_adapter.as_ref(),
                     options,
                 )
                 .await
@@ -94,7 +108,7 @@ pub async fn smart_review_command(
     };
 
     if config.smart_review_diagram {
-        match core::PRSummaryGenerator::generate_change_diagram(&diffs, adapter.as_ref()).await {
+        match core::PRSummaryGenerator::generate_change_diagram(&diffs, fast_adapter.as_ref()).await {
             Ok(Some(diagram)) => {
                 if let Some(summary) = &mut pr_summary {
                     summary.visual_diff = Some(diagram);
diff --git a/src/config.rs b/src/config.rs
index 5fce667..f63ea9e 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -20,6 +20,9 @@ pub enum ModelRole {
     Reasoning,
     /// Embedding model for RAG indexing.
     Embedding,
+    /// Fast model for lightweight LLM tasks: PR summaries, commit messages,
+    /// PR titles, diagram generation. Falls back to Weak, then Primary.
+    Fast,
 }
 
 #[derive(Debug, Clone, Serialize, Deserialize)]
@@ -51,6 +54,11 @@ pub struct Config {
     #[serde(default)]
     pub model_weak: Option<String>,
 
+    /// Fast model for lightweight LLM tasks: PR summaries, commit messages,
+    /// PR titles, diagram generation. Falls back to model_weak, then primary.
+    #[serde(default)]
+    pub model_fast: Option<String>,
+
     /// Reasoning-capable model for complex analysis and self-reflection.
     #[serde(default)]
     pub model_reasoning: Option<String>,
@@ -338,6 +346,7 @@ impl Default for Config {
         Self {
             model: default_model(),
             model_weak: None,
+            model_fast: None,
             model_reasoning: None,
             model_embedding: None,
             fallback_models: Vec::new(),
@@ -914,6 +923,11 @@ impl Config {
             ModelRole::Weak => self.model_weak.as_deref().unwrap_or(&self.model),
             ModelRole::Reasoning => self.model_reasoning.as_deref().unwrap_or(&self.model),
             ModelRole::Embedding => self.model_embedding.as_deref().unwrap_or(&self.model),
+            ModelRole::Fast => self
+                .model_fast
+                .as_deref()
+                .or(self.model_weak.as_deref())
+                .unwrap_or(&self.model),
         }
     }
 
@@ -1650,6 +1664,50 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_model_role_fast_fallback_to_primary() {
+        let config = Config {
+            model: "claude-sonnet-4-6".to_string(),
+            model_fast: None,
+            model_weak: None,
+            ..Config::default()
+        };
+        assert_eq!(config.model_for_role(ModelRole::Fast), "claude-sonnet-4-6");
+    }
+
+    #[test]
+    fn test_model_role_fast_fallback_to_weak() {
+        let config = Config {
+            model: "claude-sonnet-4-6".to_string(),
+            model_fast: None,
+            model_weak: Some("claude-haiku-4-5".to_string()),
+            ..Config::default()
+        };
+        assert_eq!(config.model_for_role(ModelRole::Fast), "claude-haiku-4-5");
+    }
+
+    #[test]
+    fn test_model_role_fast_explicit() {
+        let config = Config {
+            model: "claude-sonnet-4-6".to_string(),
+            model_fast: Some("gpt-4o-mini".to_string()),
+            model_weak: Some("claude-haiku-4-5".to_string()),
+            ..Config::default()
+        };
+        assert_eq!(config.model_for_role(ModelRole::Fast), "gpt-4o-mini");
+    }
+
+    #[test]
+    fn test_to_model_config_for_role_fast() {
+        let config = Config {
+            model: "claude-sonnet-4-6".to_string(),
+            model_fast: Some("gpt-4o-mini".to_string()),
+            ..Config::default()
+        };
+        let fast_config = config.to_model_config_for_role(ModelRole::Fast);
+        assert_eq!(fast_config.model_name, "gpt-4o-mini");
+    }
+
     #[test]
     fn test_to_model_config_for_role_uses_correct_model() {
         let config = Config {
diff --git a/src/core/interactive.rs b/src/core/interactive.rs
index fd03d90..280c16e 100644
--- a/src/core/interactive.rs
+++ b/src/core/interactive.rs
@@ -3,14 +3,13 @@ use anyhow::Result;
 use regex::Regex;
 use std::collections::HashSet;
 
-#[allow(dead_code)]
 pub struct InteractiveCommand {
     pub command: CommandType,
     pub args: Vec<String>,
+    #[allow(dead_code)] // Set by webhook handler when PR context is available
     pub context: Option<String>,
 }
 
-#[allow(dead_code)]
 #[derive(Debug, Clone, PartialEq)]
 pub enum CommandType {
     Review,
@@ -21,7 +20,6 @@ pub enum CommandType {
     Config,
 }
 
-#[allow(dead_code)]
 impl InteractiveCommand {
     pub fn parse(comment: &str) -> Option<Self> {
         let command_regex = Regex::new(r"@diffscope\s+(\w+)(?:\s+(.*))?").ok()?;
@@ -186,6 +184,10 @@ impl InteractiveCommand {
         ))
     }
 
+    pub fn help_text() -> String {
+        Self::get_help_text()
+    }
+
     fn get_help_text() -> String {
         r#"## 🤖 DiffScope Interactive Commands
 
@@ -244,6 +246,8 @@ Interactive commands respect these configurations."#
     }
 }
 
+/// Manages per-session ignore patterns from @diffscope ignore commands.
+/// Will be wired into the review pipeline's triage filter.
 #[allow(dead_code)]
 pub struct InteractiveProcessor {
     ignored_patterns: HashSet<String>,
diff --git a/src/core/offline.rs b/src/core/offline.rs
index e26228b..c0e8143 100644
--- a/src/core/offline.rs
+++ b/src/core/offline.rs
@@ -281,7 +281,6 @@ impl OfflineModelManager {
     ///
     /// Returns `Ok(Some(num_ctx))` if the model metadata contains a `num_ctx` parameter,
     /// `Ok(None)` if the parameter is not found, or `Err` if the request fails.
-    #[allow(dead_code)]
     pub async fn detect_context_window(&self, model_name: &str) -> Result<Option<usize>> {
         let url = format!("{}/api/show", self.ollama_base_url);
         let client = reqwest::Client::builder()
@@ -309,7 +308,6 @@ impl OfflineModelManager {
 ///
 /// The parameters field is a newline-separated list of key-value pairs, e.g.:
 /// `num_ctx 4096\ntemperature 0.8`
-#[allow(dead_code)]
 fn parse_num_ctx_from_params(parameters: Option<&str>) -> Option<usize> {
     let params = parameters?;
     for line in params.lines() {
diff --git a/src/plugins/plugin.rs b/src/plugins/plugin.rs
index c04bcc6..2a2d64e 100644
--- a/src/plugins/plugin.rs
+++ b/src/plugins/plugin.rs
@@ -2,23 +2,9 @@ use crate::config::PluginConfig;
 use crate::core::{Comment, LLMContextChunk, UnifiedDiff};
 use crate::plugins::{PostProcessor, PreAnalyzer};
 use anyhow::Result;
-use async_trait::async_trait;
-use std::collections::HashMap;
 use std::sync::Arc;
 
-#[async_trait]
-#[allow(dead_code)]
-pub trait Plugin: Send + Sync {
-    fn id(&self) -> &str;
-    fn name(&self) -> &str;
-    fn version(&self) -> &str;
-
-    async fn as_pre_analyzer(&self) -> Option<Box<dyn PreAnalyzer>>;
-    async fn as_post_processor(&self) -> Option<Box<dyn PostProcessor>>;
-}
-
 pub struct PluginManager {
-    _plugins: HashMap<String, Arc<dyn Plugin>>,
     pre_analyzers: Vec<Arc<dyn PreAnalyzer>>,
     post_processors: Vec<Arc<dyn PostProcessor>>,
 }
@@ -26,7 +12,6 @@ pub struct PluginManager {
 impl PluginManager {
     pub fn new() -> Self {
         Self {
-            _plugins: HashMap::new(),
             pre_analyzers: Vec::new(),
             post_processors: Vec::new(),
         }
diff --git a/src/review/pipeline.rs b/src/review/pipeline.rs
index 0e71769..9ba1ad8 100644
--- a/src/review/pipeline.rs
+++ b/src/review/pipeline.rs
@@ -251,6 +251,7 @@ async fn review_diff_content_raw_inner(
 
     let adapter: Arc<dyn adapters::llm::LLMAdapter> =
         Arc::from(adapters::llm::create_adapter(&model_config)?);
+    info!("Review adapter: {}", adapter.model_name());
 
     // Use weak model for verification pass (cheaper, faster)
     let verification_adapter: Arc<dyn adapters::llm::LLMAdapter> = {
diff --git a/src/review/verification.rs b/src/review/verification.rs
index 78ee7ad..8ba2bb6 100644
--- a/src/review/verification.rs
+++ b/src/review/verification.rs
@@ -160,10 +160,30 @@ fn parse_verification_response(content: &str, comments: &[Comment]) -> Vec<Verif
 
     for line in content.lines() {
         if let Some(caps) = FINDING_PATTERN.captures(line) {
-            let index: usize = caps.get(1).unwrap().as_str().parse().unwrap_or(0);
-            let score: u8 = caps.get(2).unwrap().as_str().parse().unwrap_or(0);
-            let accurate = caps.get(3).unwrap().as_str().to_lowercase() == "true";
-            let reason = caps.get(4).unwrap().as_str().trim().to_string();
+            let index: usize = caps
+                .get(1)
+                .expect("capture group 1 (index) must exist after regex match")
+                .as_str()
+                .parse()
+                .unwrap_or(0);
+            let score: u8 = caps
+                .get(2)
+                .expect("capture group 2 (score) must exist after regex match")
+                .as_str()
+                .parse()
+                .unwrap_or(0);
+            let accurate = caps
+                .get(3)
+                .expect("capture group 3 (accurate) must exist after regex match")
+                .as_str()
+                .to_lowercase()
+                == "true";
+            let reason = caps
+                .get(4)
+                .expect("capture group 4 (reason) must exist after regex match")
+                .as_str()
+                .trim()
+                .to_string();
 
             // Map 1-based index to comment
             if index > 0 && index <= comments.len() {
diff --git a/src/server/api.rs b/src/server/api.rs
index 89f4a0c..08dc8e8 100644
--- a/src/server/api.rs
+++ b/src/server/api.rs
@@ -577,13 +577,29 @@ pub async fn list_reviews(
     State(state): State<Arc<AppState>>,
     Query(params): Query<ListReviewsParams>,
 ) -> Json<Vec<ReviewListItem>> {
-    let reviews = state.reviews.read().await;
-    let mut list: Vec<ReviewListItem> =
-        reviews.values().map(ReviewListItem::from_session).collect();
-    list.sort_by(|a, b| b.started_at.cmp(&a.started_at));
-
     let page = params.page.unwrap_or(1).clamp(1, 10_000);
     let per_page = params.per_page.unwrap_or(20).clamp(1, 100);
+
+    // Check in-memory first
+    let mut list: Vec<ReviewListItem> = {
+        let reviews = state.reviews.read().await;
+        reviews.values().map(ReviewListItem::from_session).collect()
+    };
+
+    // Fall back to storage backend for historical reviews not in memory
+    let limit = (per_page * 2) as i64; // fetch extra to merge
+    if let Ok(stored) = state.storage.list_reviews(limit, 0).await {
+        let in_memory_ids: std::collections::HashSet<String> =
+            list.iter().map(|r| r.id.clone()).collect();
+        for session in &stored {
+            if !in_memory_ids.contains(&session.id) {
+                list.push(ReviewListItem::from_session(session));
+            }
+        }
+    }
+
+    list.sort_by(|a, b| b.started_at.cmp(&a.started_at));
+
     let start = (page - 1).saturating_mul(per_page);
     let list = if start < list.len() {
         let end = list.len().min(start.saturating_add(per_page));
@@ -595,6 +611,47 @@ pub async fn list_reviews(
     Json(list)
 }
 
+pub async fn delete_review(
+    State(state): State<Arc<AppState>>,
+    Path(id): Path<String>,
+) -> Result<Json<serde_json::Value>, StatusCode> {
+    // Remove from in-memory state
+    {
+        let mut reviews = state.reviews.write().await;
+        reviews.remove(&id);
+    }
+    // Remove from storage backend
+    if let Err(e) = state.storage.delete_review(&id).await {
+        warn!("Failed to delete review {} from storage: {}", id, e);
+    }
+    Ok(Json(serde_json::json!({ "ok": true, "deleted": id })))
+}
+
+#[derive(Deserialize)]
+pub struct PruneParams {
+    pub max_age_days: Option<i64>,
+    pub max_count: Option<usize>,
+}
+
+pub async fn prune_reviews(
+    State(state): State<Arc<AppState>>,
+    Query(params): Query<PruneParams>,
+) -> Json<serde_json::Value> {
+    let max_age_secs = params.max_age_days.unwrap_or(30) * 86400;
+    let max_count = params.max_count.unwrap_or(1000);
+
+    match state.storage.prune(max_age_secs, max_count).await {
+        Ok(pruned) => {
+            info!("Pruned {} old reviews", pruned);
+            Json(serde_json::json!({ "ok": true, "pruned": pruned }))
+        }
+        Err(e) => {
+            warn!("Prune failed: {}", e);
+            Json(serde_json::json!({ "ok": false, "error": e.to_string() }))
+        }
+    }
+}
+
 pub async fn submit_feedback(
     State(state): State<Arc<AppState>>,
     Path(id): Path<String>,
@@ -1955,8 +2012,9 @@ pub(super) async fn generate_and_store_pr_summary(
         }
     };
 
-    let model_config = config.to_model_config();
-    let adapter = match crate::adapters::llm::create_adapter(&model_config) {
+    // Use Fast model for PR summary generation (lightweight task)
+    let fast_config = config.to_model_config_for_role(crate::config::ModelRole::Fast);
+    let adapter = match crate::adapters::llm::create_adapter(&fast_config) {
         Ok(a) => a,
         Err(e) => {
             warn!(review_id = %review_id, "PR summary skipped (adapter error): {}", e);
@@ -2039,11 +2097,20 @@ pub(super) async fn post_pr_review_comments(
         if let Some(ref suggestion) = c.suggestion {
             comment_body.push_str(&format!("\n\n> **Suggestion:** {}", suggestion));
         }
+        // Calculate the line span for multi-line suggestions.
+        // GitHub requires `start_line` + `line` when a suggestion covers multiple lines.
+        let suggestion_line_span: Option<usize> = c
+            .code_suggestion
+            .as_ref()
+            .map(|cs| cs.original_code.lines().count().max(1));
+
         if let Some(ref cs) = c.code_suggestion {
-            comment_body.push_str(&format!(
-                "\n\n**Suggested fix:**\n```suggestion\n{}\n```",
-                cs.suggested_code
-            ));
+            if !cs.explanation.is_empty() {
+                comment_body.push_str(&format!("\n\n**Suggested fix:** {}", cs.explanation));
+            } else {
+                comment_body.push_str("\n\n**Suggested fix:**");
+            }
+            comment_body.push_str(&format!("\n```suggestion\n{}\n```", cs.suggested_code));
         }
 
         // Normalize file path (strip leading / or a/ b/ prefixes)
@@ -2055,12 +2122,25 @@ pub(super) async fn post_pr_review_comments(
             path
         };
 
-        inline_comments.push(serde_json::json!({
+        let mut comment_json = serde_json::json!({
             "path": path,
             "line": c.line_number,
             "side": "RIGHT",
             "body": comment_body,
-        }));
+        });
+
+        // For multi-line suggestions, set start_line so GitHub knows the full
+        // range of lines being replaced by the suggestion block.
+        if let Some(span) = suggestion_line_span {
+            if span > 1 {
+                let end_line = c.line_number + span - 1;
+                comment_json["start_line"] = serde_json::json!(c.line_number);
+                comment_json["line"] = serde_json::json!(end_line);
+                comment_json["start_side"] = serde_json::json!("RIGHT");
+            }
+        }
+
+        inline_comments.push(comment_json);
     }
 
     // Build summary body
@@ -2120,3 +2200,571 @@ pub(super) async fn post_pr_review_comments(
         Err(format!("GitHub API returned {}: {}", status, body))
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    // === ListEventsParams::into_filters tests ===
+
+    #[test]
+    fn test_into_filters_all_none() {
+        let params = ListEventsParams {
+            source: None,
+            model: None,
+            status: None,
+            time_from: None,
+            time_to: None,
+            github_repo: None,
+            limit: None,
+            offset: None,
+        };
+        let filters = params.into_filters();
+        assert_eq!(filters.source, None);
+        assert_eq!(filters.model, None);
+        assert_eq!(filters.status, None);
+        assert!(filters.time_from.is_none());
+        assert!(filters.time_to.is_none());
+        assert_eq!(filters.github_repo, None);
+        assert_eq!(filters.limit, None);
+        assert_eq!(filters.offset, None);
+    }
+
+    #[test]
+    fn test_into_filters_with_source_and_model() {
+        let params = ListEventsParams {
+            source: Some("staged".to_string()),
+            model: Some("claude-opus-4-6".to_string()),
+            status: None,
+            time_from: None,
+            time_to: None,
+            github_repo: None,
+            limit: None,
+            offset: None,
+        };
+        let filters = params.into_filters();
+        assert_eq!(filters.source, Some("staged".to_string()));
+        assert_eq!(filters.model, Some("claude-opus-4-6".to_string()));
+    }
+
+    #[test]
+    fn test_into_filters_with_status_and_github_repo() {
+        let params = ListEventsParams {
+            source: None,
+            model: None,
+            status: Some("completed".to_string()),
+            time_from: None,
+            time_to: None,
+            github_repo: Some("owner/repo".to_string()),
+            limit: Some(50),
+            offset: Some(10),
+        };
+        let filters = params.into_filters();
+        assert_eq!(filters.status, Some("completed".to_string()));
+        assert_eq!(filters.github_repo, Some("owner/repo".to_string()));
+        assert_eq!(filters.limit, Some(50));
+        assert_eq!(filters.offset, Some(10));
+    }
+
+    #[test]
+    fn test_into_filters_valid_rfc3339_time() {
+        let params = ListEventsParams {
+            source: None,
+            model: None,
+            status: None,
+            time_from: Some("2024-01-01T00:00:00Z".to_string()),
+            time_to: Some("2024-12-31T23:59:59Z".to_string()),
+            github_repo: None,
+            limit: None,
+            offset: None,
+        };
+        let filters = params.into_filters();
+        assert!(filters.time_from.is_some());
+        assert!(filters.time_to.is_some());
+        assert_eq!(
+            filters.time_from.unwrap().to_rfc3339(),
+            "2024-01-01T00:00:00+00:00"
+        );
+        assert_eq!(
+            filters.time_to.unwrap().to_rfc3339(),
+            "2024-12-31T23:59:59+00:00"
+        );
+    }
+
+    #[test]
+    fn test_into_filters_invalid_time_becomes_none() {
+        let params = ListEventsParams {
+            source: None,
+            model: None,
+            status: None,
+            time_from: Some("not-a-date".to_string()),
+            time_to: Some("also-invalid".to_string()),
+            github_repo: None,
+            limit: None,
+            offset: None,
+        };
+        let filters = params.into_filters();
+        assert!(filters.time_from.is_none());
+        assert!(filters.time_to.is_none());
+    }
+
+    #[test]
+    fn test_into_filters_all_fields_populated() {
+        let params = ListEventsParams {
+            source: Some("head".to_string()),
+            model: Some("gpt-4".to_string()),
+            status: Some("failed".to_string()),
+            time_from: Some("2025-06-01T12:00:00Z".to_string()),
+            time_to: Some("2025-06-30T12:00:00Z".to_string()),
+            github_repo: Some("haasonsaas/diffscope".to_string()),
+            limit: Some(100),
+            offset: Some(25),
+        };
+        let filters = params.into_filters();
+        assert_eq!(filters.source, Some("head".to_string()));
+        assert_eq!(filters.model, Some("gpt-4".to_string()));
+        assert_eq!(filters.status, Some("failed".to_string()));
+        assert!(filters.time_from.is_some());
+        assert!(filters.time_to.is_some());
+        assert_eq!(
+            filters.github_repo,
+            Some("haasonsaas/diffscope".to_string())
+        );
+        assert_eq!(filters.limit, Some(100));
+        assert_eq!(filters.offset, Some(25));
+    }
+
+    // === ReviewOverrides Default impl tests ===
+
+    #[test]
+    fn test_review_overrides_default() {
+        let overrides = ReviewOverrides::default();
+        assert_eq!(overrides.model, None);
+        assert_eq!(overrides.strictness, None);
+        assert_eq!(overrides.review_profile, None);
+    }
+
+    #[test]
+    fn test_review_overrides_clone() {
+        let overrides = ReviewOverrides {
+            model: Some("test-model".to_string()),
+            strictness: Some(2),
+            review_profile: Some("security".to_string()),
+        };
+        let cloned = overrides.clone();
+        assert_eq!(cloned.model, Some("test-model".to_string()));
+        assert_eq!(cloned.strictness, Some(2));
+        assert_eq!(cloned.review_profile, Some("security".to_string()));
+    }
+
+    // === mask_config_secrets tests ===
+
+    #[test]
+    fn test_mask_config_secrets_masks_api_key() {
+        let mut obj = serde_json::Map::new();
+        obj.insert(
+            "api_key".to_string(),
+            serde_json::json!("sk-secret-key-123"),
+        );
+        obj.insert("model".to_string(), serde_json::json!("gpt-4"));
+        mask_config_secrets(&mut obj);
+        assert_eq!(obj.get("api_key").unwrap(), &serde_json::json!("***"));
+        assert_eq!(obj.get("model").unwrap(), &serde_json::json!("gpt-4"));
+    }
+
+    #[test]
+    fn test_mask_config_secrets_masks_all_secret_fields() {
+        let mut obj = serde_json::Map::new();
+        obj.insert("api_key".to_string(), serde_json::json!("secret1"));
+        obj.insert("github_token".to_string(), serde_json::json!("secret2"));
+        obj.insert(
+            "github_client_secret".to_string(),
+            serde_json::json!("secret3"),
+        );
+        obj.insert(
+            "github_private_key".to_string(),
+            serde_json::json!("secret4"),
+        );
+        obj.insert(
+            "github_webhook_secret".to_string(),
+            serde_json::json!("secret5"),
+        );
+        mask_config_secrets(&mut obj);
+        assert_eq!(obj.get("api_key").unwrap(), &serde_json::json!("***"));
+        assert_eq!(obj.get("github_token").unwrap(), &serde_json::json!("***"));
+        assert_eq!(
+            obj.get("github_client_secret").unwrap(),
+            &serde_json::json!("***")
+        );
+        assert_eq!(
+            obj.get("github_private_key").unwrap(),
+            &serde_json::json!("***")
+        );
+        assert_eq!(
+            obj.get("github_webhook_secret").unwrap(),
+            &serde_json::json!("***")
+        );
+    }
+
+    #[test]
+    fn test_mask_config_secrets_does_not_mask_null_secrets() {
+        let mut obj = serde_json::Map::new();
+        obj.insert("api_key".to_string(), serde_json::Value::Null);
+        obj.insert("model".to_string(), serde_json::json!("gpt-4"));
+        mask_config_secrets(&mut obj);
+        // Null values are not strings, so they should not be masked
+        assert_eq!(obj.get("api_key").unwrap(), &serde_json::Value::Null);
+    }
+
+    #[test]
+    fn test_mask_config_secrets_no_secret_fields_present() {
+        let mut obj = serde_json::Map::new();
+        obj.insert("model".to_string(), serde_json::json!("gpt-4"));
+        obj.insert("base_url".to_string(), serde_json::json!("http://localhost"));
+        mask_config_secrets(&mut obj);
+        assert_eq!(obj.get("model").unwrap(), &serde_json::json!("gpt-4"));
+        assert_eq!(
+            obj.get("base_url").unwrap(),
+            &serde_json::json!("http://localhost")
+        );
+    }
+
+    // === mask_provider_api_keys tests ===
+
+    #[test]
+    fn test_mask_provider_api_keys_masks_nested_keys() {
+        let mut obj = serde_json::Map::new();
+        let mut providers = serde_json::Map::new();
+        let mut openai = serde_json::Map::new();
+        openai.insert("api_key".to_string(), serde_json::json!("sk-openai-key"));
+        openai.insert(
+            "base_url".to_string(),
+            serde_json::json!("https://api.openai.com"),
+        );
+        providers.insert("openai".to_string(), serde_json::Value::Object(openai));
+        obj.insert("providers".to_string(), serde_json::Value::Object(providers));
+
+        mask_provider_api_keys(&mut obj);
+
+        let providers = obj.get("providers").unwrap().as_object().unwrap();
+        let openai = providers.get("openai").unwrap().as_object().unwrap();
+        assert_eq!(openai.get("api_key").unwrap(), &serde_json::json!("***"));
+        assert_eq!(
+            openai.get("base_url").unwrap(),
+            &serde_json::json!("https://api.openai.com")
+        );
+    }
+
+    #[test]
+    fn test_mask_provider_api_keys_no_providers_field() {
+        let mut obj = serde_json::Map::new();
+        obj.insert("model".to_string(), serde_json::json!("gpt-4"));
+        mask_provider_api_keys(&mut obj);
+        // Should not panic; nothing to mask
+        assert_eq!(obj.get("model").unwrap(), &serde_json::json!("gpt-4"));
+    }
+
+    #[test]
+    fn test_mask_provider_api_keys_multiple_providers() {
+        let mut obj = serde_json::Map::new();
+        let mut providers = serde_json::Map::new();
+
+        let mut anthropic = serde_json::Map::new();
+        anthropic.insert("api_key".to_string(), serde_json::json!("ant-key"));
+        providers.insert(
+            "anthropic".to_string(),
+            serde_json::Value::Object(anthropic),
+        );
+
+        let mut ollama = serde_json::Map::new();
+        ollama.insert(
+            "base_url".to_string(),
+            serde_json::json!("http://localhost:11434"),
+        );
+        // No api_key for ollama
+        providers.insert("ollama".to_string(), serde_json::Value::Object(ollama));
+
+        obj.insert("providers".to_string(), serde_json::Value::Object(providers));
+        mask_provider_api_keys(&mut obj);
+
+        let providers = obj.get("providers").unwrap().as_object().unwrap();
+        let anthropic = providers.get("anthropic").unwrap().as_object().unwrap();
+        assert_eq!(
+            anthropic.get("api_key").unwrap(),
+            &serde_json::json!("***")
+        );
+
+        let ollama = providers.get("ollama").unwrap().as_object().unwrap();
+        assert!(ollama.get("api_key").is_none());
+    }
+
+    // === is_valid_repo_name tests ===
+
+    #[test]
+    fn test_is_valid_repo_name_standard() {
+        assert!(is_valid_repo_name("owner/repo"));
+    }
+
+    #[test]
+    fn test_is_valid_repo_name_with_hyphens_and_dots() {
+        assert!(is_valid_repo_name("my-org/my-repo.rs"));
+    }
+
+    #[test]
+    fn test_is_valid_repo_name_with_underscores() {
+        assert!(is_valid_repo_name("my_org/my_repo"));
+    }
+
+    #[test]
+    fn test_is_valid_repo_name_rejects_empty() {
+        assert!(!is_valid_repo_name(""));
+    }
+
+    #[test]
+    fn test_is_valid_repo_name_rejects_no_slash() {
+        assert!(!is_valid_repo_name("justrepo"));
+    }
+
+    #[test]
+    fn test_is_valid_repo_name_rejects_spaces() {
+        assert!(!is_valid_repo_name("owner/my repo"));
+    }
+
+    #[test]
+    fn test_is_valid_repo_name_rejects_multiple_slashes() {
+        assert!(!is_valid_repo_name("a/b/c"));
+    }
+
+    #[test]
+    fn test_is_valid_repo_name_rejects_special_chars() {
+        assert!(!is_valid_repo_name("owner/repo@v1"));
+    }
+
+    // === urlencoded tests ===
+
+    #[test]
+    fn test_urlencoded_alphanumeric() {
+        assert_eq!(urlencoded("hello123"), "hello123");
+    }
+
+    #[test]
+    fn test_urlencoded_spaces_become_plus() {
+        assert_eq!(urlencoded("hello world"), "hello+world");
+    }
+
+    #[test]
+    fn test_urlencoded_special_chars_percent_encoded() {
+        assert_eq!(urlencoded("a=b&c"), "a%3Db%26c");
+    }
+
+    #[test]
+    fn test_urlencoded_unreserved_chars_pass_through() {
+        assert_eq!(urlencoded("a-b_c.d~e"), "a-b_c.d~e");
+    }
+
+    #[test]
+    fn test_urlencoded_empty_string() {
+        assert_eq!(urlencoded(""), "");
+    }
+
+    #[test]
+    fn test_urlencoded_slash() {
+        assert_eq!(urlencoded("owner/repo"), "owner%2Frepo");
+    }
+
+    // === Request/Response serialization tests ===
+
+    #[test]
+    fn test_start_review_request_deserialize_minimal() {
+        let json = r#"{"diff_source": "head"}"#;
+        let req: StartReviewRequest = serde_json::from_str(json).unwrap();
+        assert_eq!(req.diff_source, "head");
+        assert_eq!(req.base_branch, None);
+        assert_eq!(req.diff_content, None);
+        assert_eq!(req.title, None);
+        assert_eq!(req.model, None);
+        assert_eq!(req.strictness, None);
+        assert_eq!(req.review_profile, None);
+    }
+
+    #[test]
+    fn test_start_review_request_deserialize_full() {
+        let json = r#"{
+            "diff_source": "raw",
+            "base_branch": "main",
+            "diff_content": "diff --git a/file.rs",
+            "title": "Test PR",
+            "model": "claude-opus-4-6",
+            "strictness": 3,
+            "review_profile": "security"
+        }"#;
+        let req: StartReviewRequest = serde_json::from_str(json).unwrap();
+        assert_eq!(req.diff_source, "raw");
+        assert_eq!(req.base_branch, Some("main".to_string()));
+        assert_eq!(req.diff_content, Some("diff --git a/file.rs".to_string()));
+        assert_eq!(req.title, Some("Test PR".to_string()));
+        assert_eq!(req.model, Some("claude-opus-4-6".to_string()));
+        assert_eq!(req.strictness, Some(3));
+        assert_eq!(req.review_profile, Some("security".to_string()));
+    }
+
+    #[test]
+    fn test_start_review_response_serialize() {
+        let resp = StartReviewResponse {
+            id: "abc-123".to_string(),
+            status: ReviewStatus::Pending,
+        };
+        let json = serde_json::to_value(&resp).unwrap();
+        assert_eq!(json["id"], "abc-123");
+        assert_eq!(json["status"], "Pending");
+    }
+
+    #[test]
+    fn test_feedback_request_deserialize() {
+        let json = r#"{"comment_id": "cmt-1", "action": "accept"}"#;
+        let req: FeedbackRequest = serde_json::from_str(json).unwrap();
+        assert_eq!(req.comment_id, "cmt-1");
+        assert_eq!(req.action, "accept");
+    }
+
+    #[test]
+    fn test_feedback_response_serialize() {
+        let resp = FeedbackResponse { ok: true };
+        let json = serde_json::to_value(&resp).unwrap();
+        assert_eq!(json["ok"], true);
+    }
+
+    #[test]
+    fn test_status_response_serialize() {
+        let resp = StatusResponse {
+            repo_path: "/path/to/repo".to_string(),
+            branch: Some("main".to_string()),
+            model: "gpt-4".to_string(),
+            adapter: Some("openai".to_string()),
+            base_url: None,
+            active_reviews: 2,
+        };
+        let json = serde_json::to_value(&resp).unwrap();
+        assert_eq!(json["repo_path"], "/path/to/repo");
+        assert_eq!(json["branch"], "main");
+        assert_eq!(json["model"], "gpt-4");
+        assert_eq!(json["adapter"], "openai");
+        assert!(json["base_url"].is_null());
+        assert_eq!(json["active_reviews"], 2);
+    }
+
+    #[test]
+    fn test_list_reviews_params_deserialize_empty() {
+        let json = r#"{}"#;
+        let params: ListReviewsParams = serde_json::from_str(json).unwrap();
+        assert_eq!(params.page, None);
+        assert_eq!(params.per_page, None);
+    }
+
+    #[test]
+    fn test_list_reviews_params_deserialize_with_values() {
+        let json = r#"{"page": 3, "per_page": 50}"#;
+        let params: ListReviewsParams = serde_json::from_str(json).unwrap();
+        assert_eq!(params.page, Some(3));
+        assert_eq!(params.per_page, Some(50));
+    }
+
+    #[test]
+    fn test_list_events_params_deserialize() {
+        let json = r#"{"source": "head", "limit": 100}"#;
+        let params: ListEventsParams = serde_json::from_str(json).unwrap();
+        assert_eq!(params.source, Some("head".to_string()));
+        assert_eq!(params.limit, Some(100));
+        assert_eq!(params.model, None);
+    }
+
+    #[test]
+    fn test_test_provider_request_deserialize() {
+        let json = r#"{"provider": "anthropic", "api_key": "sk-ant-123"}"#;
+        let req: TestProviderRequest = serde_json::from_str(json).unwrap();
+        assert_eq!(req.provider, "anthropic");
+        assert_eq!(req.api_key, Some("sk-ant-123".to_string()));
+        assert_eq!(req.base_url, None);
+    }
+
+    #[test]
+    fn test_test_provider_response_serialize() {
+        let resp = TestProviderResponse {
+            ok: true,
+            message: "Connected".to_string(),
+            models: vec!["model-a".to_string(), "model-b".to_string()],
+        };
+        let json = serde_json::to_value(&resp).unwrap();
+        assert_eq!(json["ok"], true);
+        assert_eq!(json["message"], "Connected");
+        assert_eq!(json["models"].as_array().unwrap().len(), 2);
+    }
+
+    #[test]
+    fn test_gh_status_response_serialize() {
+        let resp = GhStatusResponse {
+            authenticated: true,
+            username: Some("testuser".to_string()),
+            avatar_url: Some("https://example.com/avatar.png".to_string()),
+            scopes: vec!["repo".to_string(), "read:org".to_string()],
+        };
+        let json = serde_json::to_value(&resp).unwrap();
+        assert_eq!(json["authenticated"], true);
+        assert_eq!(json["username"], "testuser");
+        assert_eq!(json["scopes"].as_array().unwrap().len(), 2);
+    }
+
+    #[test]
+    fn test_gh_repo_serialize() {
+        let repo = GhRepo {
+            full_name: "owner/repo".to_string(),
+            description: Some("A test repo".to_string()),
+            language: Some("Rust".to_string()),
+            updated_at: "2024-01-01T00:00:00Z".to_string(),
+            open_prs: 5,
+            default_branch: "main".to_string(),
+            stargazers_count: 42,
+            private: false,
+        };
+        let json = serde_json::to_value(&repo).unwrap();
+        assert_eq!(json["full_name"], "owner/repo");
+        assert_eq!(json["language"], "Rust");
+        assert_eq!(json["open_prs"], 5);
+        assert_eq!(json["stargazers_count"], 42);
+        assert_eq!(json["private"], false);
+    }
+
+    #[test]
+    fn test_gh_pull_request_serialize() {
+        let pr = GhPullRequest {
+            number: 42,
+            title: "Fix bug".to_string(),
+            author: "dev".to_string(),
+            state: "open".to_string(),
+            created_at: "2024-01-01T00:00:00Z".to_string(),
+            updated_at: "2024-01-02T00:00:00Z".to_string(),
+            additions: 10,
+            deletions: 5,
+            changed_files: 3,
+            head_branch: "fix-branch".to_string(),
+            base_branch: "main".to_string(),
+            labels: vec!["bugfix".to_string()],
+            draft: false,
+        };
+        let json = serde_json::to_value(&pr).unwrap();
+        assert_eq!(json["number"], 42);
+        assert_eq!(json["title"], "Fix bug");
+        assert_eq!(json["author"], "dev");
+        assert_eq!(json["draft"], false);
+        assert_eq!(json["labels"].as_array().unwrap().len(), 1);
+    }
+
+    #[test]
+    fn test_start_pr_review_request_deserialize() {
+        let json = r#"{"repo": "owner/repo", "pr_number": 42, "post_results": true}"#;
+        let req: StartPrReviewRequest = serde_json::from_str(json).unwrap();
+        assert_eq!(req.repo, "owner/repo");
+        assert_eq!(req.pr_number, 42);
+        assert!(req.post_results);
+    }
+}
diff --git a/src/server/github.rs b/src/server/github.rs
index 61cd316..026c6af 100644
--- a/src/server/github.rs
+++ b/src/server/github.rs
@@ -331,34 +331,88 @@ pub async fn handle_webhook(
                         })?
                     };
 
-                // Fetch diff and start review
-                let diff_url =
-                    format!("https://api.github.com/repos/{}/pulls/{}", repo, pr_number,);
-                let diff_resp = state
-                    .http_client
-                    .get(&diff_url)
-                    .header("Authorization", format!("Bearer {}", auth_token))
-                    .header("Accept", "application/vnd.github.v3.diff")
-                    .header("User-Agent", "DiffScope")
-                    .send()
-                    .await
-                    .map_err(|e| {
+                // Determine whether to fetch an incremental diff or the full PR diff.
+                // For "synchronize" events, if we have a previously reviewed SHA,
+                // fetch only the commits since that SHA via the compare API.
+                let pr_key = format!("{}#{}", repo, pr_number);
+                let last_reviewed_sha = if action == "synchronize" {
+                    AppState::get_last_reviewed_sha(&state, &pr_key).await
+                } else {
+                    None
+                };
+
+                let (diff_content, is_incremental) =
+                    if let Some(ref base_sha) = last_reviewed_sha {
+                        // Incremental: fetch only new commits since last review
+                        info!(
+                            repo = %repo,
+                            pr = pr_number,
+                            base_sha = %base_sha,
+                            head_sha = %head_sha,
+                            "Fetching incremental diff (push-by-push)"
+                        );
+                        let compare_url = format!(
+                            "https://api.github.com/repos/{}/compare/{}...{}",
+                            repo, base_sha, head_sha,
+                        );
+                        let compare_resp = state
+                            .http_client
+                            .get(&compare_url)
+                            .header("Authorization", format!("Bearer {}", auth_token))
+                            .header("Accept", "application/vnd.github.v3.diff")
+                            .header("User-Agent", "DiffScope")
+                            .send()
+                            .await
+                            .map_err(|e| {
+                                (
+                                    StatusCode::BAD_GATEWAY,
+                                    format!("Failed to fetch incremental diff: {}", e),
+                                )
+                            })?;
+
+                        if compare_resp.status().is_success() {
+                            let content = compare_resp.text().await.unwrap_or_default();
+                            if content.trim().is_empty() {
+                                // No changes between SHAs (e.g. force push to same content);
+                                // fall through to full diff below
+                                info!(
+                                    repo = %repo,
+                                    pr = pr_number,
+                                    "Incremental diff empty, falling back to full PR diff"
+                                );
+                                (
+                                    fetch_full_pr_diff(&state, &auth_token, &repo, pr_number)
+                                        .await?,
+                                    false,
+                                )
+                            } else {
+                                (content, true)
+                            }
+                        } else {
+                            // Compare API failed (e.g. force push rewrote history);
+                            // fall back to full PR diff
+                            let status = compare_resp.status();
+                            let body = compare_resp.text().await.unwrap_or_default();
+                            warn!(
+                                repo = %repo,
+                                pr = pr_number,
+                                status = %status,
+                                "Incremental compare failed ({}), falling back to full PR diff: {}",
+                                status,
+                                body,
+                            );
+                            (
+                                fetch_full_pr_diff(&state, &auth_token, &repo, pr_number).await?,
+                                false,
+                            )
+                        }
+                    } else {
+                        // No previous review or "opened" action — full PR diff
                         (
-                            StatusCode::BAD_GATEWAY,
-                            format!("Failed to fetch diff: {}", e),
+                            fetch_full_pr_diff(&state, &auth_token, &repo, pr_number).await?,
+                            false,
                         )
-                    })?;
-
-                if !diff_resp.status().is_success() {
-                    let status = diff_resp.status();
-                    let body = diff_resp.text().await.unwrap_or_default();
-                    return Err((
-                        StatusCode::BAD_GATEWAY,
-                        format!("GitHub returned {}: {}", status, body),
-                    ));
-                }
-
-                let diff_content = diff_resp.text().await.unwrap_or_default();
+                    };
 
                 let review_id = uuid::Uuid::new_v4().to_string();
                 let diff_source = format!("pr:{}#{}", repo, pr_number);
@@ -399,6 +453,7 @@ pub async fn handle_webhook(
                             head_sha,
                             pr_title,
                             auth_token,
+                            is_incremental,
                         },
                     )
                     .await;
@@ -408,9 +463,79 @@ pub async fn handle_webhook(
                     "ok": true,
                     "action": "review_started",
                     "review_id": review_id,
+                    "incremental": is_incremental,
                 })));
             }
         }
+        "issue_comment" => {
+            let payload: serde_json::Value = serde_json::from_str(&body)
+                .map_err(|e| (StatusCode::BAD_REQUEST, format!("Invalid JSON: {}", e)))?;
+
+            let action = payload["action"].as_str().unwrap_or("");
+            let comment_body = payload["comment"]["body"].as_str().unwrap_or("");
+
+            // Only process new comments that contain @diffscope commands
+            if action == "created" {
+                if let Some(cmd) = crate::core::interactive::InteractiveCommand::parse(comment_body)
+                {
+                    let repo = payload["repository"]["full_name"]
+                        .as_str()
+                        .unwrap_or("")
+                        .to_string();
+                    let issue_number = payload["issue"]["number"].as_u64().unwrap_or(0) as u32;
+
+                    info!(
+                        repo = %repo,
+                        issue = issue_number,
+                        command = ?cmd.command,
+                        "Processing @diffscope command"
+                    );
+
+                    // Execute the command
+                    let response_body = match cmd.command {
+                        crate::core::interactive::CommandType::Help => {
+                            crate::core::interactive::InteractiveCommand::help_text()
+                        }
+                        _ => {
+                            // Create adapter from server config for LLM commands
+                            let config = state.config.read().await;
+                            let model_config = config.to_model_config();
+                            drop(config);
+
+                            match crate::adapters::llm::create_adapter(&model_config) {
+                                Ok(adapter) => match cmd.execute(adapter.as_ref(), None).await {
+                                    Ok(result) => result,
+                                    Err(e) => format!("Command failed: {}", e),
+                                },
+                                Err(e) => format!("Failed to create adapter: {}", e),
+                            }
+                        }
+                    };
+
+                    // Post response comment if we have a token
+                    if let Some(ref auth) = token {
+                        let comment_url = format!(
+                            "https://api.github.com/repos/{}/issues/{}/comments",
+                            repo, issue_number
+                        );
+                        let _ = state
+                            .http_client
+                            .post(&comment_url)
+                            .header("Authorization", format!("Bearer {}", auth))
+                            .header("User-Agent", "DiffScope")
+                            .json(&serde_json::json!({ "body": response_body }))
+                            .send()
+                            .await;
+                    }
+
+                    return Ok(Json(serde_json::json!({
+                        "ok": true,
+                        "action": "command_processed",
+                        "command": format!("{:?}", cmd.command),
+                    })));
+                }
+            }
+        }
         "ping" => {
             info!("GitHub webhook ping received");
             return Ok(Json(serde_json::json!({ "ok": true, "action": "pong" })));
@@ -680,9 +805,51 @@ struct WebhookReviewParams {
     head_sha: String,
     pr_title: String,
     auth_token: String,
+    /// Whether this review covers only incremental changes (push-by-push delta).
+    is_incremental: bool,
 }
 
-#[tracing::instrument(name = "github.webhook_review", skip(state, params), fields(review_id = %params.review_id, repo = %params.repo, pr_number = params.pr_number, diff_bytes = params.diff_content.len()))]
+/// Fetch the full PR diff from the GitHub API.
+async fn fetch_full_pr_diff(
+    state: &Arc<AppState>,
+    auth_token: &str,
+    repo: &str,
+    pr_number: u32,
+) -> Result<String, (StatusCode, String)> {
+    let diff_url = format!("https://api.github.com/repos/{}/pulls/{}", repo, pr_number);
+    let diff_resp = state
+        .http_client
+        .get(&diff_url)
+        .header("Authorization", format!("Bearer {}", auth_token))
+        .header("Accept", "application/vnd.github.v3.diff")
+        .header("User-Agent", "DiffScope")
+        .send()
+        .await
+        .map_err(|e| {
+            (
+                StatusCode::BAD_GATEWAY,
+                format!("Failed to fetch diff: {}", e),
+            )
+        })?;
+
+    if !diff_resp.status().is_success() {
+        let status = diff_resp.status();
+        let body = diff_resp.text().await.unwrap_or_default();
+        return Err((
+            StatusCode::BAD_GATEWAY,
+            format!("GitHub returned {}: {}", status, body),
+        ));
+    }
+
+    diff_resp.text().await.map_err(|e| {
+        (
+            StatusCode::BAD_GATEWAY,
+            format!("Failed to read diff body: {}", e),
+        )
+    })
+}
+
+#[tracing::instrument(name = "github.webhook_review", skip(state, params), fields(review_id = %params.review_id, repo = %params.repo, pr_number = params.pr_number, diff_bytes = params.diff_content.len(), incremental = params.is_incremental))]
 async fn run_webhook_review(state: Arc<AppState>, params: WebhookReviewParams) {
     let WebhookReviewParams {
         review_id,
@@ -692,6 +859,7 @@ async fn run_webhook_review(state: Arc<AppState>, params: WebhookReviewParams) {
         head_sha,
         pr_title,
         auth_token,
+        is_incremental,
     } = params;
     use crate::core::comment::CommentSynthesizer;
 
@@ -745,6 +913,9 @@ async fn run_webhook_review(state: Arc<AppState>, params: WebhookReviewParams) {
             event,
         )
         .await;
+        // Record the reviewed SHA for future incremental reviews
+        let pr_key = format!("{}#{}", repo, pr_number);
+        AppState::record_reviewed_sha(&state, &pr_key, &head_sha).await;
         AppState::save_reviews_async(&state);
         return;
     }
@@ -787,13 +958,17 @@ async fn run_webhook_review(state: Arc<AppState>, params: WebhookReviewParams) {
             }
 
             // Create Check Run
+            let incremental_tag = if is_incremental { " (incremental)" } else { "" };
             let check_title = if pr_title.is_empty() {
                 format!(
-                    "DiffScope: {}/{}",
-                    summary.total_comments, summary.overall_score
+                    "DiffScope{}: {}/{}",
+                    incremental_tag, summary.total_comments, summary.overall_score
                 )
             } else {
-                format!("DiffScope: {} — {:.1}/10", pr_title, summary.overall_score)
+                format!(
+                    "DiffScope{}: {} — {:.1}/10",
+                    incremental_tag, pr_title, summary.overall_score
+                )
             };
             let _ = create_check_run(
                 &state.http_client,
@@ -857,6 +1032,18 @@ async fn run_webhook_review(state: Arc<AppState>, params: WebhookReviewParams) {
             AppState::complete_review(&state, &review_id, comments, summary, files_reviewed, event)
                 .await;
 
+            // Record the reviewed SHA for future incremental reviews
+            let pr_key = format!("{}#{}", repo, pr_number);
+            AppState::record_reviewed_sha(&state, &pr_key, &head_sha).await;
+            if is_incremental {
+                info!(
+                    repo = %repo,
+                    pr = pr_number,
+                    head_sha = %head_sha,
+                    "Incremental review completed, updated last reviewed SHA"
+                );
+            }
+
             // Generate AI-powered PR summary and post it as a comment if enabled
             if let Some(ref cfg) = summary_config {
                 super::api::generate_and_store_pr_summary(&state, &review_id, &diff_content, cfg)
diff --git a/src/server/mod.rs b/src/server/mod.rs
index 5c93e4d..ab680a4 100644
--- a/src/server/mod.rs
+++ b/src/server/mod.rs
@@ -19,6 +19,7 @@ use std::sync::Arc;
 use tower_http::cors::CorsLayer;
 
 use crate::config::Config;
+use tracing::{info, warn};
 
 #[derive(RustEmbed)]
 #[folder = "web/dist"]
@@ -77,7 +78,7 @@ pub async fn start_server(config: Config, host: &str, port: u16) -> anyhow::Resu
         .filter_map(|s| match s.parse() {
             Ok(v) => Some(v),
             Err(e) => {
-                eprintln!("Warning: failed to parse CORS origin '{}': {}", s, e);
+                warn!("Failed to parse CORS origin '{}': {}", s, e);
                 None
             }
         })
@@ -99,7 +100,9 @@ pub async fn start_server(config: Config, host: &str, port: u16) -> anyhow::Resu
         .route("/events", get(api::list_events))
         .route("/events/stats", get(api::get_event_stats))
         .route("/review/{id}", get(api::get_review))
+        .route("/review/{id}", delete(api::delete_review))
         .route("/review/{id}/feedback", post(api::submit_feedback))
+        .route("/reviews/prune", post(api::prune_reviews))
         .route("/doctor", get(api::get_doctor))
         .route("/config", get(api::get_config))
         .route("/config", put(api::update_config))
@@ -123,8 +126,8 @@ pub async fn start_server(config: Config, host: &str, port: u16) -> anyhow::Resu
         .layer(cors);
 
     let addr: SocketAddr = format!("{}:{}", host, port).parse()?;
-    eprintln!("DiffScope server running at http://{}", addr);
-    eprintln!("Press Ctrl+C to stop");
+    info!("DiffScope server running at http://{}", addr);
+    info!("Press Ctrl+C to stop");
 
     let listener = tokio::net::TcpListener::bind(addr).await?;
     axum::serve(listener, app).await?;
diff --git a/src/server/state.rs b/src/server/state.rs
index 8302a43..25776b1 100644
--- a/src/server/state.rs
+++ b/src/server/state.rs
@@ -3,7 +3,7 @@ use std::collections::HashMap;
 use std::path::PathBuf;
 use std::sync::Arc;
 use tokio::sync::RwLock;
-use tracing::info;
+use tracing::{error, info, warn};
 
 use crate::config::Config;
 use crate::core::comment::{Comment, ReviewSummary};
@@ -163,6 +163,9 @@ pub struct AppState {
     pub http_client: reqwest::Client,
     /// Semaphore to limit concurrent review tasks.
     pub review_semaphore: Arc<tokio::sync::Semaphore>,
+    /// Tracks the last reviewed head SHA per PR, keyed by "owner/repo#pr_number".
+    /// Used for incremental (push-by-push) reviews.
+    pub last_reviewed_shas: Arc<RwLock<HashMap<String, String>>>,
 }
 
 impl AppState {
@@ -224,6 +227,7 @@ impl AppState {
             config_path,
             http_client,
             review_semaphore: Arc::new(tokio::sync::Semaphore::new(MAX_CONCURRENT_REVIEWS)),
+            last_reviewed_shas: Arc::new(RwLock::new(HashMap::new())),
         };
 
         Ok(state)
@@ -248,7 +252,7 @@ impl AppState {
                 match serde_json::to_string_pretty(&stripped) {
                     Ok(j) => j,
                     Err(e) => {
-                        eprintln!("Failed to serialize reviews: {}", e);
+                        error!("Failed to serialize reviews: {}", e);
                         return;
                     }
                 }
@@ -257,18 +261,18 @@ impl AppState {
 
             if let Some(parent) = state.storage_path.parent() {
                 if let Err(e) = tokio::fs::create_dir_all(parent).await {
-                    eprintln!("Failed to create storage directory: {}", e);
+                    error!("Failed to create storage directory: {}", e);
                     return;
                 }
             }
             // Write to a temp file then rename for atomic writes
             let tmp_path = state.storage_path.with_extension("json.tmp");
             if let Err(e) = tokio::fs::write(&tmp_path, &json).await {
-                eprintln!("Failed to write reviews temp file: {}", e);
+                error!("Failed to write reviews temp file: {}", e);
                 return;
             }
             if let Err(e) = tokio::fs::rename(&tmp_path, &state.storage_path).await {
-                eprintln!("Failed to rename reviews file: {}", e);
+                error!("Failed to rename reviews file: {}", e);
                 let _ = tokio::fs::remove_file(&tmp_path).await;
             }
         })
@@ -283,7 +287,7 @@ impl AppState {
                 match serde_json::to_string_pretty(&*config) {
                     Ok(j) => j,
                     Err(e) => {
-                        eprintln!("Failed to serialize config: {}", e);
+                        error!("Failed to serialize config: {}", e);
                         return;
                     }
                 }
@@ -291,17 +295,17 @@ impl AppState {
 
             if let Some(parent) = state.config_path.parent() {
                 if let Err(e) = tokio::fs::create_dir_all(parent).await {
-                    eprintln!("Failed to create config directory: {}", e);
+                    error!("Failed to create config directory: {}", e);
                     return;
                 }
             }
             let tmp_path = state.config_path.with_extension("json.tmp");
             if let Err(e) = tokio::fs::write(&tmp_path, &json).await {
-                eprintln!("Failed to write config: {}", e);
+                error!("Failed to write config: {}", e);
                 return;
             }
             if let Err(e) = tokio::fs::rename(&tmp_path, &state.config_path).await {
-                eprintln!("Failed to rename config file: {}", e);
+                error!("Failed to rename config file: {}", e);
                 let _ = tokio::fs::remove_file(&tmp_path).await;
             }
         })
@@ -314,16 +318,16 @@ impl AppState {
         match std::fs::read_to_string(path) {
             Ok(data) => match serde_json::from_str::<HashMap<String, ReviewSession>>(&data) {
                 Ok(loaded) => {
-                    eprintln!("Loaded {} reviews from disk", loaded.len());
+                    info!("Loaded {} reviews from disk", loaded.len());
                     loaded
                 }
                 Err(e) => {
-                    eprintln!("Failed to parse reviews.json: {}", e);
+                    warn!("Failed to parse reviews.json: {}", e);
                     HashMap::new()
                 }
             },
             Err(e) => {
-                eprintln!("Failed to read reviews.json: {}", e);
+                warn!("Failed to read reviews.json: {}", e);
                 HashMap::new()
             }
         }
@@ -410,6 +414,20 @@ impl AppState {
             }
         }
     }
+
+    /// Record the head SHA after a successful review for a PR.
+    /// The key is formatted as "owner/repo#pr_number".
+    pub async fn record_reviewed_sha(state: &Arc<AppState>, pr_key: &str, head_sha: &str) {
+        let mut shas = state.last_reviewed_shas.write().await;
+        shas.insert(pr_key.to_string(), head_sha.to_string());
+    }
+
+    /// Look up the last reviewed head SHA for a PR.
+    /// Returns `None` if this PR has never been reviewed.
+    pub async fn get_last_reviewed_sha(state: &Arc<AppState>, pr_key: &str) -> Option<String> {
+        let shas = state.last_reviewed_shas.read().await;
+        shas.get(pr_key).cloned()
+    }
 }
 
 /// Lightweight view of a review session for list endpoints (no comments/diff/event).
@@ -925,6 +943,7 @@ mod tests {
                 storage_path: storage_path.clone(),
                 config_path,
                 review_semaphore: Arc::new(tokio::sync::Semaphore::new(5)),
+                last_reviewed_shas: Arc::new(tokio::sync::RwLock::new(HashMap::new())),
             });
             let handle = AppState::save_reviews_async(&state);
             // The handle should be awaitable and complete successfully
@@ -998,4 +1017,109 @@ mod tests {
             assert_eq!(sem.available_permits(), 1);
         });
     }
+
+    #[test]
+    fn test_record_and_get_last_reviewed_sha() {
+        let rt = tokio::runtime::Builder::new_current_thread()
+            .enable_all()
+            .build()
+            .unwrap();
+        rt.block_on(async {
+            let dir = tempfile::tempdir().unwrap();
+            let storage_path = dir.path().join("reviews.json");
+            let config_path = dir.path().join("config.json");
+            let json_backend = crate::server::storage_json::JsonStorageBackend::new(&storage_path);
+            let state = Arc::new(AppState {
+                config: Arc::new(tokio::sync::RwLock::new(crate::config::Config::default())),
+                repo_path: dir.path().to_path_buf(),
+                reviews: Arc::new(tokio::sync::RwLock::new(HashMap::new())),
+                storage: Arc::new(json_backend),
+                http_client: reqwest::Client::new(),
+                storage_path,
+                config_path,
+                review_semaphore: Arc::new(tokio::sync::Semaphore::new(5)),
+                last_reviewed_shas: Arc::new(tokio::sync::RwLock::new(HashMap::new())),
+            });
+
+            let pr_key = "owner/repo#42";
+
+            // Initially no SHA recorded
+            assert!(AppState::get_last_reviewed_sha(&state, pr_key)
+                .await
+                .is_none());
+
+            // Record a SHA
+            AppState::record_reviewed_sha(&state, pr_key, "abc123").await;
+            assert_eq!(
+                AppState::get_last_reviewed_sha(&state, pr_key)
+                    .await
+                    .as_deref(),
+                Some("abc123"),
+            );
+
+            // Update the SHA
+            AppState::record_reviewed_sha(&state, pr_key, "def456").await;
+            assert_eq!(
+                AppState::get_last_reviewed_sha(&state, pr_key)
+                    .await
+                    .as_deref(),
+                Some("def456"),
+            );
+
+            // Different PR key is independent
+            let other_key = "owner/repo#99";
+            assert!(AppState::get_last_reviewed_sha(&state, other_key)
+                .await
+                .is_none());
+        });
+    }
+
+    #[test]
+    fn test_last_reviewed_shas_multiple_prs() {
+        let rt = tokio::runtime::Builder::new_current_thread()
+            .enable_all()
+            .build()
+            .unwrap();
+        rt.block_on(async {
+            let dir = tempfile::tempdir().unwrap();
+            let storage_path = dir.path().join("reviews.json");
+            let config_path = dir.path().join("config.json");
+            let json_backend = crate::server::storage_json::JsonStorageBackend::new(&storage_path);
+            let state = Arc::new(AppState {
+                config: Arc::new(tokio::sync::RwLock::new(crate::config::Config::default())),
+                repo_path: dir.path().to_path_buf(),
+                reviews: Arc::new(tokio::sync::RwLock::new(HashMap::new())),
+                storage: Arc::new(json_backend),
+                http_client: reqwest::Client::new(),
+                storage_path,
+                config_path,
+                review_semaphore: Arc::new(tokio::sync::Semaphore::new(5)),
+                last_reviewed_shas: Arc::new(tokio::sync::RwLock::new(HashMap::new())),
+            });
+
+            // Record SHAs for multiple PRs across different repos
+            AppState::record_reviewed_sha(&state, "org/repo-a#1", "sha_a1").await;
+            AppState::record_reviewed_sha(&state, "org/repo-a#2", "sha_a2").await;
+            AppState::record_reviewed_sha(&state, "org/repo-b#1", "sha_b1").await;
+
+            assert_eq!(
+                AppState::get_last_reviewed_sha(&state, "org/repo-a#1")
+                    .await
+                    .as_deref(),
+                Some("sha_a1"),
+            );
+            assert_eq!(
+                AppState::get_last_reviewed_sha(&state, "org/repo-a#2")
+                    .await
+                    .as_deref(),
+                Some("sha_a2"),
+            );
+            assert_eq!(
+                AppState::get_last_reviewed_sha(&state, "org/repo-b#1")
+                    .await
+                    .as_deref(),
+                Some("sha_b1"),
+            );
+        });
+    }
 }
diff --git a/src/server/storage.rs b/src/server/storage.rs
index cde9c54..1830d5f 100644
--- a/src/server/storage.rs
+++ b/src/server/storage.rs
@@ -70,7 +70,6 @@ pub struct DailyCount {
 }
 
 /// Abstract storage backend - implemented by JSON file and PostgreSQL
-#[allow(dead_code)]
 #[async_trait]
 pub trait StorageBackend: Send + Sync {
     // Reviews
diff --git a/src/server/storage_json.rs b/src/server/storage_json.rs
index d30d4de..ec2b4a8 100644
--- a/src/server/storage_json.rs
+++ b/src/server/storage_json.rs
@@ -2,6 +2,7 @@ use async_trait::async_trait;
 use std::collections::HashMap;
 use std::path::PathBuf;
 use tokio::sync::RwLock;
+use tracing::{error, info, warn};
 
 use super::state::{ReviewEvent, ReviewSession, ReviewStatus};
 use super::storage::{
@@ -31,16 +32,16 @@ impl JsonStorageBackend {
         match std::fs::read_to_string(path) {
             Ok(data) => match serde_json::from_str::<HashMap<String, ReviewSession>>(&data) {
                 Ok(loaded) => {
-                    eprintln!("Loaded {} reviews from disk", loaded.len());
+                    info!("Loaded {} reviews from disk", loaded.len());
                     loaded
                 }
                 Err(e) => {
-                    eprintln!("Failed to parse reviews.json: {}", e);
+                    warn!("Failed to parse reviews.json: {}", e);
                     HashMap::new()
                 }
             },
             Err(e) => {
-                eprintln!("Failed to read reviews.json: {}", e);
+                warn!("Failed to read reviews.json: {}", e);
                 HashMap::new()
             }
         }
@@ -60,7 +61,7 @@ impl JsonStorageBackend {
             match serde_json::to_string_pretty(&stripped) {
                 Ok(j) => j,
                 Err(e) => {
-                    eprintln!("Failed to serialize reviews: {}", e);
+                    error!("Failed to serialize reviews: {}", e);
                     return;
                 }
             }
@@ -68,17 +69,17 @@ impl JsonStorageBackend {
 
         if let Some(parent) = self.storage_path.parent() {
             if let Err(e) = tokio::fs::create_dir_all(parent).await {
-                eprintln!("Failed to create storage directory: {}", e);
+                error!("Failed to create storage directory: {}", e);
                 return;
             }
         }
         let tmp_path = self.storage_path.with_extension("json.tmp");
         if let Err(e) = tokio::fs::write(&tmp_path, &json).await {
-            eprintln!("Failed to write reviews temp file: {}", e);
+            error!("Failed to write reviews temp file: {}", e);
             return;
         }
         if let Err(e) = tokio::fs::rename(&tmp_path, &self.storage_path).await {
-            eprintln!("Failed to rename reviews file: {}", e);
+            error!("Failed to rename reviews file: {}", e);
             let _ = tokio::fs::remove_file(&tmp_path).await;
         }
     }
diff --git a/src/server/storage_pg.rs b/src/server/storage_pg.rs
index d62bcac..3ed65f0 100644
--- a/src/server/storage_pg.rs
+++ b/src/server/storage_pg.rs
@@ -733,3 +733,184 @@ impl EventRow {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::core::comment::{Category, FixEffort, Severity};
+    use crate::server::state::ReviewStatus;
+
+    // === parse_status tests ===
+
+    #[test]
+    fn test_parse_status_running() {
+        assert_eq!(parse_status("Running"), ReviewStatus::Running);
+    }
+
+    #[test]
+    fn test_parse_status_complete() {
+        assert_eq!(parse_status("Complete"), ReviewStatus::Complete);
+    }
+
+    #[test]
+    fn test_parse_status_failed() {
+        assert_eq!(parse_status("Failed"), ReviewStatus::Failed);
+    }
+
+    #[test]
+    fn test_parse_status_pending() {
+        assert_eq!(parse_status("Pending"), ReviewStatus::Pending);
+    }
+
+    #[test]
+    fn test_parse_status_unknown_defaults_to_pending() {
+        assert_eq!(parse_status("unknown"), ReviewStatus::Pending);
+    }
+
+    #[test]
+    fn test_parse_status_empty_defaults_to_pending() {
+        assert_eq!(parse_status(""), ReviewStatus::Pending);
+    }
+
+    #[test]
+    fn test_parse_status_case_sensitive() {
+        // Lowercase variants should not match; they fall through to default
+        assert_eq!(parse_status("running"), ReviewStatus::Pending);
+        assert_eq!(parse_status("complete"), ReviewStatus::Pending);
+        assert_eq!(parse_status("failed"), ReviewStatus::Pending);
+    }
+
+    // === parse_severity tests ===
+
+    #[test]
+    fn test_parse_severity_error() {
+        assert_eq!(parse_severity("Error"), Severity::Error);
+    }
+
+    #[test]
+    fn test_parse_severity_warning() {
+        assert_eq!(parse_severity("Warning"), Severity::Warning);
+    }
+
+    #[test]
+    fn test_parse_severity_info() {
+        assert_eq!(parse_severity("Info"), Severity::Info);
+    }
+
+    #[test]
+    fn test_parse_severity_suggestion() {
+        assert_eq!(parse_severity("Suggestion"), Severity::Suggestion);
+    }
+
+    #[test]
+    fn test_parse_severity_unknown_defaults_to_suggestion() {
+        assert_eq!(parse_severity("unknown"), Severity::Suggestion);
+    }
+
+    #[test]
+    fn test_parse_severity_empty_defaults_to_suggestion() {
+        assert_eq!(parse_severity(""), Severity::Suggestion);
+    }
+
+    #[test]
+    fn test_parse_severity_case_sensitive() {
+        assert_eq!(parse_severity("error"), Severity::Suggestion);
+        assert_eq!(parse_severity("WARNING"), Severity::Suggestion);
+    }
+
+    // === parse_category tests ===
+
+    #[test]
+    fn test_parse_category_bug() {
+        assert_eq!(parse_category("Bug"), Category::Bug);
+    }
+
+    #[test]
+    fn test_parse_category_security() {
+        assert_eq!(parse_category("Security"), Category::Security);
+    }
+
+    #[test]
+    fn test_parse_category_performance() {
+        assert_eq!(parse_category("Performance"), Category::Performance);
+    }
+
+    #[test]
+    fn test_parse_category_style() {
+        assert_eq!(parse_category("Style"), Category::Style);
+    }
+
+    #[test]
+    fn test_parse_category_documentation() {
+        assert_eq!(parse_category("Documentation"), Category::Documentation);
+    }
+
+    #[test]
+    fn test_parse_category_best_practice() {
+        assert_eq!(parse_category("BestPractice"), Category::BestPractice);
+    }
+
+    #[test]
+    fn test_parse_category_maintainability() {
+        assert_eq!(parse_category("Maintainability"), Category::Maintainability);
+    }
+
+    #[test]
+    fn test_parse_category_testing() {
+        assert_eq!(parse_category("Testing"), Category::Testing);
+    }
+
+    #[test]
+    fn test_parse_category_architecture() {
+        assert_eq!(parse_category("Architecture"), Category::Architecture);
+    }
+
+    #[test]
+    fn test_parse_category_unknown_defaults_to_architecture() {
+        assert_eq!(parse_category("unknown"), Category::Architecture);
+    }
+
+    #[test]
+    fn test_parse_category_empty_defaults_to_architecture() {
+        assert_eq!(parse_category(""), Category::Architecture);
+    }
+
+    #[test]
+    fn test_parse_category_case_sensitive() {
+        assert_eq!(parse_category("bug"), Category::Architecture);
+        assert_eq!(parse_category("SECURITY"), Category::Architecture);
+    }
+
+    // === parse_fix_effort tests ===
+
+    #[test]
+    fn test_parse_fix_effort_low() {
+        assert_eq!(parse_fix_effort("Low"), FixEffort::Low);
+    }
+
+    #[test]
+    fn test_parse_fix_effort_medium() {
+        assert_eq!(parse_fix_effort("Medium"), FixEffort::Medium);
+    }
+
+    #[test]
+    fn test_parse_fix_effort_high() {
+        assert_eq!(parse_fix_effort("High"), FixEffort::High);
+    }
+
+    #[test]
+    fn test_parse_fix_effort_unknown_defaults_to_low() {
+        assert_eq!(parse_fix_effort("unknown"), FixEffort::Low);
+    }
+
+    #[test]
+    fn test_parse_fix_effort_empty_defaults_to_low() {
+        assert_eq!(parse_fix_effort(""), FixEffort::Low);
+    }
+
+    #[test]
+    fn test_parse_fix_effort_case_sensitive() {
+        assert_eq!(parse_fix_effort("medium"), FixEffort::Low);
+        assert_eq!(parse_fix_effort("HIGH"), FixEffort::Low);
+    }
+}

From 00d1c9b7dc2020121ab917a572796039e8556227 Mon Sep 17 00:00:00 2001
From: Jonathan Haas <jonathan@haas.holdings>
Date: Wed, 11 Mar 2026 12:48:07 -0700
Subject: [PATCH 2/2] =?UTF-8?q?fix:=20address=20review=20findings=20?=
 =?UTF-8?q?=E2=80=94=20pagination,=20prune=20safety,=20redundant=20adapter?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- Fix list_reviews pagination: fetch enough rows from storage to cover
  the requested page, not just 2x per_page
- Fix prune_reviews: also prune in-memory state, clamp max_age_days
  and max_count to >= 1 to prevent negative values deleting all reviews
- Avoid redundant adapter allocation in smart_review when fast model
  matches primary model — reuse existing adapter reference
- Run cargo fmt to fix CI formatting failure

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---
 src/commands/smart_review.rs |  15 +++--
 src/server/api.rs            |  29 ++++++---
 src/server/github.rs         | 116 +++++++++++++++++------------------
 3 files changed, 85 insertions(+), 75 deletions(-)

diff --git a/src/commands/smart_review.rs b/src/commands/smart_review.rs
index 4fc4cf7..49db279 100644
--- a/src/commands/smart_review.rs
+++ b/src/commands/smart_review.rs
@@ -63,18 +63,21 @@ pub async fn smart_review_command(
 
     let adapter = adapters::llm::create_adapter(&model_config)?;
 
-    // Use Fast model for PR summary and diagram generation (lightweight tasks)
+    // Use Fast model for PR summary and diagram generation (lightweight tasks).
+    // Only create a separate adapter if model_fast differs from the primary model.
     let fast_config = config.to_model_config_for_role(config::ModelRole::Fast);
-    let fast_adapter: Box<dyn adapters::llm::LLMAdapter> =
+    let separate_fast_adapter: Option<Box<dyn adapters::llm::LLMAdapter>> =
         if fast_config.model_name != model_config.model_name {
             info!(
                 "Using fast model '{}' for PR summary/diagram",
                 fast_config.model_name
             );
-            adapters::llm::create_adapter(&fast_config)?
+            Some(adapters::llm::create_adapter(&fast_config)?)
         } else {
-            adapters::llm::create_adapter(&model_config)?
+            None
         };
+    let summary_adapter: &dyn adapters::llm::LLMAdapter =
+        separate_fast_adapter.as_deref().unwrap_or(adapter.as_ref());
 
     let mut all_comments = Vec::new();
     let mut pr_summary = if config.smart_review_summary {
@@ -86,7 +89,7 @@ pub async fn smart_review_command(
                 match core::PRSummaryGenerator::generate_summary_with_options(
                     &diffs,
                     &git,
-                    fast_adapter.as_ref(),
+                    summary_adapter,
                     options,
                 )
                 .await
@@ -108,7 +111,7 @@ pub async fn smart_review_command(
     };
 
     if config.smart_review_diagram {
-        match core::PRSummaryGenerator::generate_change_diagram(&diffs, fast_adapter.as_ref()).await {
+        match core::PRSummaryGenerator::generate_change_diagram(&diffs, summary_adapter).await {
             Ok(Some(diagram)) => {
                 if let Some(summary) = &mut pr_summary {
                     summary.visual_diff = Some(diagram);
diff --git a/src/server/api.rs b/src/server/api.rs
index 08dc8e8..db5533d 100644
--- a/src/server/api.rs
+++ b/src/server/api.rs
@@ -587,7 +587,7 @@ pub async fn list_reviews(
     };
 
     // Fall back to storage backend for historical reviews not in memory
-    let limit = (per_page * 2) as i64; // fetch extra to merge
+    let limit = (page * per_page + per_page) as i64; // fetch enough to cover requested page
     if let Ok(stored) = state.storage.list_reviews(limit, 0).await {
         let in_memory_ids: std::collections::HashSet<String> =
             list.iter().map(|r| r.id.clone()).collect();
@@ -637,8 +637,11 @@ pub async fn prune_reviews(
     State(state): State<Arc<AppState>>,
     Query(params): Query<PruneParams>,
 ) -> Json<serde_json::Value> {
-    let max_age_secs = params.max_age_days.unwrap_or(30) * 86400;
-    let max_count = params.max_count.unwrap_or(1000);
+    let max_age_secs = params.max_age_days.unwrap_or(30).max(1) * 86400;
+    let max_count = params.max_count.unwrap_or(1000).max(1);
+
+    // Prune in-memory state first
+    AppState::prune_old_reviews(&state).await;
 
     match state.storage.prune(max_age_secs, max_count).await {
         Ok(pruned) => {
@@ -2420,7 +2423,10 @@ mod tests {
     fn test_mask_config_secrets_no_secret_fields_present() {
         let mut obj = serde_json::Map::new();
         obj.insert("model".to_string(), serde_json::json!("gpt-4"));
-        obj.insert("base_url".to_string(), serde_json::json!("http://localhost"));
+        obj.insert(
+            "base_url".to_string(),
+            serde_json::json!("http://localhost"),
+        );
         mask_config_secrets(&mut obj);
         assert_eq!(obj.get("model").unwrap(), &serde_json::json!("gpt-4"));
         assert_eq!(
@@ -2442,7 +2448,10 @@ mod tests {
             serde_json::json!("https://api.openai.com"),
         );
         providers.insert("openai".to_string(), serde_json::Value::Object(openai));
-        obj.insert("providers".to_string(), serde_json::Value::Object(providers));
+        obj.insert(
+            "providers".to_string(),
+            serde_json::Value::Object(providers),
+        );
 
         mask_provider_api_keys(&mut obj);
 
@@ -2484,15 +2493,15 @@ mod tests {
         // No api_key for ollama
         providers.insert("ollama".to_string(), serde_json::Value::Object(ollama));
 
-        obj.insert("providers".to_string(), serde_json::Value::Object(providers));
+        obj.insert(
+            "providers".to_string(),
+            serde_json::Value::Object(providers),
+        );
         mask_provider_api_keys(&mut obj);
 
         let providers = obj.get("providers").unwrap().as_object().unwrap();
         let anthropic = providers.get("anthropic").unwrap().as_object().unwrap();
-        assert_eq!(
-            anthropic.get("api_key").unwrap(),
-            &serde_json::json!("***")
-        );
+        assert_eq!(anthropic.get("api_key").unwrap(), &serde_json::json!("***"));
 
         let ollama = providers.get("ollama").unwrap().as_object().unwrap();
         assert!(ollama.get("api_key").is_none());
diff --git a/src/server/github.rs b/src/server/github.rs
index 026c6af..238e5a1 100644
--- a/src/server/github.rs
+++ b/src/server/github.rs
@@ -341,78 +341,76 @@ pub async fn handle_webhook(
                     None
                 };
 
-                let (diff_content, is_incremental) =
-                    if let Some(ref base_sha) = last_reviewed_sha {
-                        // Incremental: fetch only new commits since last review
-                        info!(
-                            repo = %repo,
-                            pr = pr_number,
-                            base_sha = %base_sha,
-                            head_sha = %head_sha,
-                            "Fetching incremental diff (push-by-push)"
-                        );
-                        let compare_url = format!(
-                            "https://api.github.com/repos/{}/compare/{}...{}",
-                            repo, base_sha, head_sha,
-                        );
-                        let compare_resp = state
-                            .http_client
-                            .get(&compare_url)
-                            .header("Authorization", format!("Bearer {}", auth_token))
-                            .header("Accept", "application/vnd.github.v3.diff")
-                            .header("User-Agent", "DiffScope")
-                            .send()
-                            .await
-                            .map_err(|e| {
-                                (
-                                    StatusCode::BAD_GATEWAY,
-                                    format!("Failed to fetch incremental diff: {}", e),
-                                )
-                            })?;
-
-                        if compare_resp.status().is_success() {
-                            let content = compare_resp.text().await.unwrap_or_default();
-                            if content.trim().is_empty() {
-                                // No changes between SHAs (e.g. force push to same content);
-                                // fall through to full diff below
-                                info!(
-                                    repo = %repo,
-                                    pr = pr_number,
-                                    "Incremental diff empty, falling back to full PR diff"
-                                );
-                                (
-                                    fetch_full_pr_diff(&state, &auth_token, &repo, pr_number)
-                                        .await?,
-                                    false,
-                                )
-                            } else {
-                                (content, true)
-                            }
-                        } else {
-                            // Compare API failed (e.g. force push rewrote history);
-                            // fall back to full PR diff
-                            let status = compare_resp.status();
-                            let body = compare_resp.text().await.unwrap_or_default();
-                            warn!(
+                let (diff_content, is_incremental) = if let Some(ref base_sha) = last_reviewed_sha {
+                    // Incremental: fetch only new commits since last review
+                    info!(
+                        repo = %repo,
+                        pr = pr_number,
+                        base_sha = %base_sha,
+                        head_sha = %head_sha,
+                        "Fetching incremental diff (push-by-push)"
+                    );
+                    let compare_url = format!(
+                        "https://api.github.com/repos/{}/compare/{}...{}",
+                        repo, base_sha, head_sha,
+                    );
+                    let compare_resp = state
+                        .http_client
+                        .get(&compare_url)
+                        .header("Authorization", format!("Bearer {}", auth_token))
+                        .header("Accept", "application/vnd.github.v3.diff")
+                        .header("User-Agent", "DiffScope")
+                        .send()
+                        .await
+                        .map_err(|e| {
+                            (
+                                StatusCode::BAD_GATEWAY,
+                                format!("Failed to fetch incremental diff: {}", e),
+                            )
+                        })?;
+
+                    if compare_resp.status().is_success() {
+                        let content = compare_resp.text().await.unwrap_or_default();
+                        if content.trim().is_empty() {
+                            // No changes between SHAs (e.g. force push to same content);
+                            // fall through to full diff below
+                            info!(
                                 repo = %repo,
                                 pr = pr_number,
-                                status = %status,
-                                "Incremental compare failed ({}), falling back to full PR diff: {}",
-                                status,
-                                body,
+                                "Incremental diff empty, falling back to full PR diff"
                             );
                             (
                                 fetch_full_pr_diff(&state, &auth_token, &repo, pr_number).await?,
                                 false,
                             )
+                        } else {
+                            (content, true)
                         }
                     } else {
-                        // No previous review or "opened" action — full PR diff
+                        // Compare API failed (e.g. force push rewrote history);
+                        // fall back to full PR diff
+                        let status = compare_resp.status();
+                        let body = compare_resp.text().await.unwrap_or_default();
+                        warn!(
+                            repo = %repo,
+                            pr = pr_number,
+                            status = %status,
+                            "Incremental compare failed ({}), falling back to full PR diff: {}",
+                            status,
+                            body,
+                        );
                         (
                             fetch_full_pr_diff(&state, &auth_token, &repo, pr_number).await?,
                             false,
                         )
-                    };
+                    }
+                } else {
+                    // No previous review or "opened" action — full PR diff
+                    (
+                        fetch_full_pr_diff(&state, &auth_token, &repo, pr_number).await?,
+                        false,
+                    )
+                };
 
                 let review_id = uuid::Uuid::new_v4().to_string();
                 let diff_source = format!("pr:{}#{}", repo, pr_number);