fix: address code review issues and add KBA drafter e2e tests

Fixes:
- Fix CSV folder case mismatch (CSV -> csv) in app.py and operations.py
- Remove duplicate get_ticket_by_incident_id method in csv_data.py
- Replace inefficient len(session.exec().all()) with SQL COUNT(*) in kba_service.py
- Replace hardcoded placeholder credentials with env var lookups in kba_service.py
- Fix scheduler swallowing exceptions (remove bare raise, return None)
- Add settings reload at start of each scheduler run to fix race condition
- Add generation_warnings field to surface search questions failures to users
- Add schema migration for generation_warnings column

Tests:
- Add 19 Playwright e2e tests for KBA Drafter feature covering:
  page load, navigation, LLM health status, draft generation,
  draft display, draft list, editing, review workflow,
  duplicate handling, and backend API integration

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: abossard

backend/app.py

@@ -779,7 +779,7 @@ async def get_qa_tickets():
 _csv_ticket_service = get_csv_ticket_service()
 
 # Load CSV data on startup (using relative path from backend folder)
-_csv_data_path = Path(__file__).parent.parent / "CSV" / "data.csv"
+_csv_data_path = Path(__file__).parent.parent / "csv" / "data.csv"
 if _csv_data_path.exists():
     _csv_loaded = _csv_ticket_service.load_csv(_csv_data_path)
     print(f"📊 Loaded {_csv_loaded} tickets from CSV")

backend/csv_data.py

@@ -467,15 +467,14 @@ def get_ticket(self, ticket_id: UUID) -> Optional[Ticket]:
         return self._tickets.get(ticket_id)
 
     def get_ticket_by_incident_id(self, incident_id: str) -> Optional[Ticket]:
-        """Get ticket by INC number (e.g. INC000016349327)."""
-        return self._tickets_by_incident_id.get(incident_id)
-    
-    def get_ticket_by_incident_id(self, incident_id: str) -> Optional[Ticket]:
-        """
-        Get ticket by Incident ID (e.g., INC000016349815).
+        """Get ticket by INC number (e.g. INC000016349327).
         
-        Uses deterministic UUID generation to find ticket.
+        Uses direct dictionary lookup first, falls back to UUID-based lookup.
         """
+        ticket = self._tickets_by_incident_id.get(incident_id)
+        if ticket is not None:
+            return ticket
+        # Fallback: deterministic UUID generation
         ticket_uuid = generate_uuid_from_incident_id(incident_id)
         return self._tickets.get(ticket_uuid)
 

backend/kba_models.py

@@ -113,6 +113,12 @@ class KBADraft(BaseModel):
         description="User search queries - how users might search for this KBA"
     )
 
+    # Generation warnings (e.g. search questions generation failed)
+    generation_warnings: list[str] = Field(
+        default_factory=list,
+        description="Warnings from the generation process (partial failures)"
+    )
+    
     # Guidelines used during generation
     guidelines_used: list[str] = Field(default_factory=list, description="Guideline categories used")
 
@@ -270,6 +276,7 @@ class KBADraftTable(SQLModel, table=True):
     tags: list[str] = SQLField(sa_column=Column(JSON))
     related_tickets: list[str] = SQLField(sa_column=Column(JSON))
     search_questions: list[str] = SQLField(sa_column=Column(JSON))
+    generation_warnings: list[str] = SQLField(sa_column=Column(JSON, default=[]))
     guidelines_used: list[str] = SQLField(sa_column=Column(JSON))
 
     # Metadata

backend/kba_service.py

@@ -16,11 +16,13 @@
 """
 
 import logging
+import os
 from datetime import datetime
 from time import perf_counter
 from typing import Optional, Union
 from uuid import UUID, uuid4
 
+from sqlalchemy import func
 from sqlmodel import Session, select, desc
 
 from csv_data import get_csv_ticket_service
@@ -221,12 +223,14 @@ async def generate_draft(self, create_req: KBADraftCreate) -> KBADraft:
                 extra={"draft_id": str(draft.id), "ticket_id": str(create_req.ticket_id)}
             )
         except Exception as e:
-            # Log error but continue - search questions are optional
             logger.warning(
                 f"Failed to generate search questions: {str(e)}",
                 extra={"draft_id": str(draft.id), "error": str(e)}
             )
             draft.search_questions = []
+            draft.generation_warnings.append(
+                f"Search questions generation failed: {str(e)}"
+            )
 
         # 7. Save to database
         draft_table = self._draft_to_table(draft)
@@ -557,8 +561,9 @@ def list_drafts(self, filters: KBADraftFilter) -> KBADraftListResponse:
         if filters.incident_id:
             statement = statement.where(KBADraftTable.incident_id == filters.incident_id)
 
-        # Count total
-        total = len(self.session.exec(statement).all())
+        # Count total using SQL COUNT(*)
+        count_statement = select(func.count()).select_from(statement.subquery())
+        total = self.session.exec(count_statement).one()
 
         # Apply pagination
         statement = statement.offset(filters.offset).limit(filters.limit)
@@ -981,23 +986,31 @@ def _get_adapter_config(self, target_system: str) -> dict:
                 "create_categories": True
             },
             "sharepoint": {
-                "site_url": "https://example.sharepoint.com/sites/KB",
-                "client_id": "...",  # TODO: Load from env
-                "client_secret": "..."  # TODO: Load from env
+                "site_url": os.environ.get("KB_SHAREPOINT_SITE_URL", ""),
+                "client_id": os.environ.get("KB_SHAREPOINT_CLIENT_ID", ""),
+                "client_secret": os.environ.get("KB_SHAREPOINT_CLIENT_SECRET", ""),
             },
             "itsm": {
-                "instance_url": "https://example.service-now.com",
-                "username": "...",  # TODO: Load from env
-                "password": "..."  # TODO: Load from env
+                "instance_url": os.environ.get("KB_ITSM_INSTANCE_URL", ""),
+                "username": os.environ.get("KB_ITSM_USERNAME", ""),
+                "password": os.environ.get("KB_ITSM_PASSWORD", ""),
             },
             "confluence": {
-                "base_url": "https://example.atlassian.net",
-                "username": "...",  # TODO: Load from env
-                "api_token": "..."  # TODO: Load from env
+                "base_url": os.environ.get("KB_CONFLUENCE_BASE_URL", ""),
+                "username": os.environ.get("KB_CONFLUENCE_USERNAME", ""),
+                "api_token": os.environ.get("KB_CONFLUENCE_API_TOKEN", ""),
             }
         }
 
-        return configs.get(target_system, {})
+        config = configs.get(target_system, {})
+        if target_system != "file" and config:
+            missing = [k for k, v in config.items() if not v]
+            if missing:
+                raise NotImplementedError(
+                    f"Adapter '{target_system}' requires environment variables: "
+                    f"{', '.join(f'KB_{target_system.upper()}_{k.upper()}' for k in missing)}"
+                )
+        return config
 
     def _draft_to_table(self, draft: KBADraft) -> KBADraftTable:
         """Convert KBADraft to KBADraftTable for persistence"""
@@ -1024,6 +1037,7 @@ def _draft_to_table(self, draft: KBADraft) -> KBADraftTable:
             tags=draft.tags,
             related_tickets=draft.related_tickets,
             search_questions=draft.search_questions if draft.search_questions is not None else [],
+            generation_warnings=draft.generation_warnings if draft.generation_warnings is not None else [],
             guidelines_used=draft.guidelines_used,
             status=draft.status.value,
             created_at=draft.created_at,
@@ -1055,6 +1069,7 @@ def _table_to_draft(self, table: KBADraftTable) -> KBADraft:
             tags=table.tags,
             related_tickets=table.related_tickets,
             search_questions=table.search_questions if hasattr(table, 'search_questions') and table.search_questions is not None else [],
+            generation_warnings=table.generation_warnings if hasattr(table, 'generation_warnings') and table.generation_warnings is not None else [],
             guidelines_used=table.guidelines_used,
             status=KBADraftStatus(table.status),
             created_at=table.created_at,

backend/operations.py

@@ -71,6 +71,13 @@ def _migrate_kba_schema(engine) -> None:
                 "ALTER TABLE kba_drafts ADD COLUMN is_auto_generated INTEGER DEFAULT 0"
             ))
             session.commit()
+        
+        # Check if generation_warnings column exists
+        if 'generation_warnings' not in columns:
+            session.exec(text(
+                "ALTER TABLE kba_drafts ADD COLUMN generation_warnings TEXT DEFAULT '[]'"
+            ))
+            session.commit()
 
 
 def _get_kba_session() -> Session:
@@ -134,7 +141,7 @@ def _ensure_csv_loaded() -> None:
     if _csv_loaded:
         return
 
-    default_csv_path = Path(__file__).resolve().parents[1] / "CSV" / "data.csv"
+    default_csv_path = Path(__file__).resolve().parents[1] / "csv" / "data.csv"
     if default_csv_path.exists():
         try:
             _csv_service.load_csv(default_csv_path)

backend/scheduler.py

@@ -90,8 +90,18 @@ async def run_now(self) -> dict:
         return result.model_dump()
 
     async def _run_auto_generation(self):
-        """Internal method to run auto-generation"""
+        """Internal method to run auto-generation.
+        
+        Reloads settings on each run to pick up any changes.
+        Catches all exceptions to prevent APScheduler from silently swallowing them.
+        """
         try:
+            # Reload settings to pick up any changes since last run
+            settings = self.auto_gen_service.get_settings()
+            if not settings.enabled:
+                logger.info("Auto-generation is disabled, skipping run")
+                return None
+            
             result = await self.auto_gen_service.run_auto_generation()
 
             if result.success:
@@ -109,7 +119,7 @@ async def _run_auto_generation(self):
 
         except Exception as e:
             logger.error(f"Auto-generation failed with exception: {e}", exc_info=True)
-            raise
+            return None
 
     @staticmethod
     def _parse_time(time_str: str) -> tuple[int, int]: