Fix bugs found during code audit

Server fixes:
- Remove unwrap() on reqwest client builder in doctor handler (panic risk)
- Use saturating_mul for pagination to prevent integer overflow on large page values
- Cap page/per_page to sane maximums (10000/100)
- Validate branch names against allowed characters to prevent injection
- Return 404 for unmatched /api/ routes instead of SPA HTML fallback
- Use tokio::fs instead of blocking std::fs in async save task
- Log persistence errors to stderr instead of silently ignoring

Helm chart fixes:
- Fix GPU resources creating duplicate 'limits' key in ollama-deployment
  (now properly merges nvidia.com/gpu into existing limits block)
- Move PVC YAML separator inside conditional to avoid dangling '---'
  when first PVC is not rendered

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: haasonsaas

charts/diffscope/templates/ollama-deployment.yaml

@@ -52,12 +52,17 @@ spec:
               port: http
             initialDelaySeconds: 10
             periodSeconds: 10
+          {{- if .Values.ollama.gpu.enabled }}
           resources:
-            {{- toYaml .Values.ollama.resources | nindent 12 }}
-            {{- if .Values.ollama.gpu.enabled }}
+            requests:
+              {{- toYaml .Values.ollama.resources.requests | nindent 14 }}
             limits:
+              {{- toYaml .Values.ollama.resources.limits | nindent 14 }}
               nvidia.com/gpu: {{ .Values.ollama.gpu.count }}
-            {{- end }}
+          {{- else }}
+          resources:
+            {{- toYaml .Values.ollama.resources | nindent 12 }}
+          {{- end }}
           volumeMounts:
             - name: ollama-data
               mountPath: /root/.ollama

charts/diffscope/templates/pvc.yaml

@@ -19,8 +19,8 @@ spec:
     requests:
       storage: {{ .Values.persistence.size }}
 {{- end }}
----
 {{- if and .Values.ollama.enabled .Values.ollama.persistence.enabled (not .Values.ollama.persistence.existingClaim) }}
+---
 apiVersion: v1
 kind: PersistentVolumeClaim
 metadata:

src/server/api.rs

@@ -145,6 +145,19 @@ async fn run_review_task(
     let config = state.config.read().await.clone();
     let repo_path = state.repo_path.clone();
 
+    // Validate branch name to prevent injection
+    if let Some(ref branch) = base_branch {
+        if !branch.chars().all(|c| c.is_alphanumeric() || matches!(c, '/' | '-' | '_' | '.')) {
+            let mut reviews = state.reviews.write().await;
+            if let Some(session) = reviews.get_mut(&review_id) {
+                session.status = ReviewStatus::Failed;
+                session.error = Some("Invalid branch name".to_string());
+                session.completed_at = Some(current_timestamp());
+            }
+            return;
+        }
+    }
+
     // Get the diff content based on source
     let diff_result = match diff_source.as_str() {
         "staged" => get_diff_from_git(&repo_path, "staged", None),
@@ -301,11 +314,12 @@ pub async fn list_reviews(
     list.sort_by(|a, b| b.started_at.cmp(&a.started_at));
 
     // Apply pagination
-    let page = params.page.unwrap_or(1).max(1);
-    let per_page = params.per_page.unwrap_or(20).max(1);
-    let start = (page - 1) * per_page;
+    let page = params.page.unwrap_or(1).max(1).min(10_000);
+    let per_page = params.per_page.unwrap_or(20).max(1).min(100);
+    let start = (page - 1).saturating_mul(per_page);
     let list = if start < list.len() {
-        list[start..list.len().min(start + per_page)].to_vec()
+        let end = list.len().min(start.saturating_add(per_page));
+        list[start..end].to_vec()
     } else {
         Vec::new()
     };
@@ -344,11 +358,6 @@ pub async fn get_doctor(State(state): State<Arc<AppState>>) -> Json<serde_json::
         .clone()
         .unwrap_or_else(|| "http://localhost:11434".to_string());
 
-    let client = reqwest::Client::builder()
-        .timeout(std::time::Duration::from_secs(5))
-        .build()
-        .unwrap();
-
     let mut result = serde_json::json!({
         "config": {
             "model": config.model,
@@ -363,6 +372,14 @@ pub async fn get_doctor(State(state): State<Arc<AppState>>) -> Json<serde_json::
         "recommended_model": null,
     });
 
+    let client = match reqwest::Client::builder()
+        .timeout(std::time::Duration::from_secs(5))
+        .build()
+    {
+        Ok(c) => c,
+        Err(_) => return Json(result),
+    };
+
     // Check Ollama
     let ollama_url = format!("{}/api/tags", base_url);
     if let Ok(resp) = client.get(&ollama_url).send().await {

src/server/mod.rs

@@ -21,6 +21,11 @@ struct WebAssets;
 async fn serve_embedded(uri: axum::http::Uri) -> Response {
     let path = uri.path().trim_start_matches('/');
 
+    // Don't serve SPA for unmatched /api/ routes
+    if path.starts_with("api/") {
+        return (StatusCode::NOT_FOUND, "Not found").into_response();
+    }
+
     // Try exact path first, then fall back to index.html (SPA routing)
     let (file, serve_path) = if path.is_empty() {
         (WebAssets::get("index.html"), "index.html")

src/server/state.rs

@@ -93,10 +93,18 @@ impl AppState {
             drop(reviews);
 
             if let Some(parent) = state.storage_path.parent() {
-                let _ = std::fs::create_dir_all(parent);
+                if let Err(e) = tokio::fs::create_dir_all(parent).await {
+                    eprintln!("Failed to create storage directory: {}", e);
+                    return;
+                }
             }
-            if let Ok(json) = serde_json::to_string_pretty(&stripped) {
-                let _ = std::fs::write(&state.storage_path, json);
+            match serde_json::to_string_pretty(&stripped) {
+                Ok(json) => {
+                    if let Err(e) = tokio::fs::write(&state.storage_path, json).await {
+                        eprintln!("Failed to persist reviews: {}", e);
+                    }
+                }
+                Err(e) => eprintln!("Failed to serialize reviews: {}", e),
             }
         });
     }