fix(classify): LLM fallback bypasses cascade short-circuit (closes #99)

src/classify/classifier.rs

@@ -248,18 +248,45 @@ impl ClassificationEngine {
             return r;
         }
 
-        if let Some(llm) = &self.llm {
-            if let Some(mut r) = llm.classify(message).await {
-                r.top_level = self.taxonomy.resolve(&r.category);
-                return r;
-            }
+        if let Some(r) = self.llm_classify_only(message).await {
+            return r;
         }
 
         let mut fallback = ClassificationResult::unclassified();
         fallback.ticket_id = RegexMatcher::extract_ticket_id(message);
         fallback
     }
 
+    /// Invoke the LLM tier directly, bypassing tiers 0–3.5.
+    ///
+    /// Returns `None` when the LLM tier is not configured, no API key is
+    /// reachable, or the underlying request fails. The pipeline-level LLM
+    /// fallback uses this to route low-confidence catch-all verdicts to the
+    /// LLM without re-running `classify_sync` (which would short-circuit on
+    /// the same low-confidence verdict that triggered the fallback).
+    ///
+    /// Backfills `ticket_id` from the message text — the LLM verdict
+    /// itself does not surface ticket IDs, and without this the pipeline's
+    /// overwrite-guard would otherwise drop a ticket reference carried by
+    /// the original tier-1-3 verdict when the LLM result wins.
+    pub async fn llm_classify_only(&self, message: &str) -> Option<ClassificationResult> {
+        let llm = self.llm.as_ref()?;
+        let mut r = llm.classify(message).await?;
+        r.top_level = self.taxonomy.resolve(&r.category);
+        if r.ticket_id.is_none() {
+            r.ticket_id = RegexMatcher::extract_ticket_id(message);
+        }
+        Some(r)
+    }
+
+    /// `Some(true)` when the LLM tier is enabled and has a reachable API
+    /// key, `Some(false)` when it is enabled but unconfigured, `None` when
+    /// the tier is disabled entirely. Callers can warn at startup when the
+    /// middle case occurs to avoid silent misconfiguration.
+    pub fn llm_has_api_key(&self) -> Option<bool> {
+        self.llm.as_ref().map(LlmClassifier::has_api_key)
+    }
+
     /// Classify a batch of `(message, is_merge)` pairs in parallel using
     /// Rayon (tiers 1–3 only). Entries where no tier matched are returned
     /// as [`ClassificationResult::unclassified`].

src/classify/mod.rs

@@ -842,6 +842,78 @@ mod tests {
         );
     }
 
+    #[tokio::test]
+    async fn llm_classify_only_returns_none_when_disabled() {
+        // Issue #99 regression: `llm_classify_only` must NOT fall back to
+        // `classify_sync`. When `use_llm: false`, calling it for a message
+        // that the regex tier would classify as `maintenance / 0.3` (the
+        // catch-all) must still return `None` — otherwise the pipeline's
+        // overwrite-guard would see the same low-confidence verdict back
+        // and the LLM tier would never run.
+        let engine = ClassificationEngine::new(
+            rules::default_rules(),
+            ClassificationEngineConfig {
+                use_llm: false,
+                ..Default::default()
+            },
+        )
+        .expect("engine");
+        assert!(engine
+            .llm_classify_only("xyzzy plugh frobnicate")
+            .await
+            .is_none());
+        assert_eq!(engine.llm_has_api_key(), None);
+    }
+
+    /// Panic-safe env-var save/restore (mirrors the pattern in
+    /// `core::config::validator::tests::EnvVarGuard`). Without this, a
+    /// failing assertion between `remove_var` and the restore would leak
+    /// the cleared state to other parallel tests in the same binary.
+    struct EnvVarGuard {
+        name: &'static str,
+        original: Option<String>,
+    }
+
+    impl EnvVarGuard {
+        fn remove(name: &'static str) -> Self {
+            let original = std::env::var(name).ok();
+            // SAFETY: 2024-edition env mutation; cleanup guaranteed by Drop.
+            unsafe { std::env::remove_var(name) };
+            Self { name, original }
+        }
+    }
+
+    impl Drop for EnvVarGuard {
+        fn drop(&mut self) {
+            // SAFETY: see `EnvVarGuard::remove`.
+            unsafe {
+                match self.original.as_deref() {
+                    Some(v) => std::env::set_var(self.name, v),
+                    None => std::env::remove_var(self.name),
+                }
+            }
+        }
+    }
+
+    #[tokio::test]
+    async fn llm_has_api_key_signals_misconfiguration() {
+        // Issue #99 follow-up: when use_llm is on but no env key is set,
+        // the engine should expose `Some(false)` so the pipeline can warn
+        // at startup instead of silently producing no LLM verdicts.
+        let _guard = EnvVarGuard::remove("OPENAI_API_KEY");
+
+        let engine = ClassificationEngine::new(
+            rules::default_rules(),
+            ClassificationEngineConfig {
+                use_llm: true,
+                llm_provider: "openai".to_string(),
+                ..Default::default()
+            },
+        )
+        .expect("engine");
+        assert_eq!(engine.llm_has_api_key(), Some(false));
+    }
+
     #[tokio::test]
     async fn pipeline_runs_against_in_memory_db() {
         use crate::core::config::Config;

src/classify/pipeline.rs

@@ -180,6 +180,18 @@ impl ClassificationPipeline {
         //    back, so the borrow checker doesn't see mutable refs into
         //    `results` while futures are in flight.
         if engine.config().use_llm {
+            // Single startup-time diagnostic when the LLM tier is on but no
+            // credential is reachable. Without this, the fallback would emit
+            // a warn-per-commit ("did not improve confidence") that obscures
+            // the real misconfiguration.
+            if matches!(engine.llm_has_api_key(), Some(false)) {
+                warn!(
+                    "LLM tier enabled but no API key resolved \
+                     (OPENAI_API_KEY / OPENROUTER_API_KEY unset); \
+                     fallback will short-circuit silently"
+                );
+            }
+
             let fallback_threshold = self
                 .config
                 .classification
@@ -218,8 +230,16 @@ impl ClassificationPipeline {
             let pb_ref = &pb;
             let new_results: Vec<(usize, ClassificationResult, f64)> =
                 futures::stream::iter(pending.into_iter().map(
-                    |(idx, message, is_merge, original_conf)| async move {
-                        let r = engine_ref.classify(&message, is_merge).await;
+                    |(idx, message, _is_merge, original_conf)| async move {
+                        // Direct LLM dispatch — calling `engine_ref.classify`
+                        // here would re-run `classify_sync` first and short-
+                        // circuit on the same low-confidence tier-1-3 verdict
+                        // that triggered the fallback, so the LLM tier would
+                        // never be reached (issue #99).
+                        let r = engine_ref
+                            .llm_classify_only(&message)
+                            .await
+                            .unwrap_or_else(ClassificationResult::unclassified);
                         pb_ref.inc(1);
                         (idx, r, original_conf)
                     },

src/classify/tiers/llm.rs

@@ -187,6 +187,17 @@ impl LlmClassifier {
         self
     }
 
+    /// Whether this classifier has a usable credential.
+    ///
+    /// `true` when an API key was resolved (for HTTP providers) or when a
+    /// Bedrock backend is wired up. `false` indicates `classify` would
+    /// short-circuit to `None` for HTTP providers — the pipeline uses this
+    /// at startup to emit a single warning instead of silently producing
+    /// "LLM fallback did not improve confidence" for every commit.
+    pub fn has_api_key(&self) -> bool {
+        self.bedrock.is_some() || self.api_key.is_some()
+    }
+
     /// Classify `message` by calling the LLM.
     ///
     /// Returns `None` if the LLM is disabled (no API key), the request
@@ -317,3 +328,88 @@ struct LlmVerdict {
 fn default_confidence() -> f64 {
     0.5
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use wiremock::matchers::{method, path};
+    use wiremock::{Mock, MockServer, ResponseTemplate};
+
+    #[test]
+    fn has_api_key_reflects_key_state() {
+        let with_key = LlmClassifier::new("gpt-4o-mini", Some("sk-test".to_string()));
+        assert!(with_key.has_api_key());
+        let without_key = LlmClassifier::new("gpt-4o-mini", None);
+        assert!(!without_key.has_api_key());
+    }
+
+    #[tokio::test]
+    async fn classify_returns_none_without_api_key() {
+        let llm = LlmClassifier::new("gpt-4o-mini", None);
+        assert!(llm.classify("feat: anything").await.is_none());
+    }
+
+    /// Regression for the hive-review finding in PR #(issue 99): the raw
+    /// `LlmClassifier` always sets `ticket_id: None`. The engine wrapper
+    /// (`Engine::llm_classify_only`) backfills it via regex extraction.
+    /// This test pins the raw classifier's contract so any future change
+    /// that starts surfacing `ticket_id` from the LLM verdict prompts the
+    /// engine wrapper to be revisited (so we don't double-backfill).
+    #[tokio::test]
+    async fn classify_does_not_set_ticket_id() {
+        let server = MockServer::start().await;
+        let body = serde_json::json!({
+            "choices": [{
+                "message": {
+                    "content": "{\"category\": \"bugfix\", \
+                                 \"subcategory\": null, \
+                                 \"confidence\": 0.8}"
+                }
+            }]
+        });
+        Mock::given(method("POST"))
+            .and(path("/v1/chat/completions"))
+            .respond_with(ResponseTemplate::new(200).set_body_json(body))
+            .mount(&server)
+            .await;
+
+        let llm = LlmClassifier::new("gpt-4o-mini", Some("sk-test".to_string()))
+            .with_endpoint(format!("{}/v1/chat/completions", server.uri()));
+        let r = llm
+            .classify("fix: handle null in PROJ-1234 endpoint")
+            .await
+            .expect("LLM verdict");
+        assert_eq!(r.ticket_id, None);
+    }
+
+    #[tokio::test]
+    async fn classify_dispatches_to_endpoint_when_keyed() {
+        // Regression: issue #99 — when the pipeline asks the LLM tier
+        // directly, an HTTP call must happen even for messages a regex
+        // tier would have caught. This test verifies the raw classifier
+        // hits its configured endpoint and returns the LLM verdict.
+        let server = MockServer::start().await;
+        let body = serde_json::json!({
+            "choices": [{
+                "message": {
+                    "content": "{\"category\": \"feature\", \
+                                 \"subcategory\": \"new-auth\", \
+                                 \"confidence\": 0.91}"
+                }
+            }]
+        });
+        Mock::given(method("POST"))
+            .and(path("/v1/chat/completions"))
+            .respond_with(ResponseTemplate::new(200).set_body_json(body))
+            .mount(&server)
+            .await;
+
+        let llm = LlmClassifier::new("gpt-4o-mini", Some("sk-test".to_string()))
+            .with_endpoint(format!("{}/v1/chat/completions", server.uri()));
+        let r = llm.classify("chore: bump deps").await.expect("LLM verdict");
+        assert_eq!(r.category, "feature");
+        assert_eq!(r.subcategory.as_deref(), Some("new-auth"));
+        assert!((r.confidence - 0.91).abs() < 1e-6);
+        assert_eq!(r.method, ClassificationMethod::LlmFallback);
+    }
+}