legco_ai_assistant/.plans/phase1_debug_plan.md

Phase 1 Backend Debug Plan

Source: Code review of Phase 1 implementation Scope: Issues found across all Phase 1 backend files Date: 2026-04-22 Status: ✅ Complete — 41 passed, 2 skipped (python-docx not installed)

---

Objective

Fix all issues discovered during Phase 1 code review before starting Phase 2. Issues are prioritized by severity — P0 items will cause production failures or silent correctness bugs.

---

Issue Summary

Priority	Count	Category
P0 Critical	4	Missing embeddings, blocking async, per-request instantiation, silent failures
P1 High	4	Business logic in routers, scope deviation, token explosion, missing model files
P2 Medium	5	Hardcoded config, missing types, empty tests, dead dependency
P3 Low	4	Global mutable state, type ignores, unused imports, dead assertions
Total	17

---

P0 — Critical (will cause production failures)

Issue 1: No embeddings are being generated — ChromaDB uses default embedder

Files: services/rag.py, core/database.py, core/config.py Severity: Silent functional bug — query quality depends on wrong embedding model

Problem: The plan calls for embedding with qwen/qwen3-embedding-4b via embedding_model and embedding_base_url in config. Nothing uses them. get_or_create_collection() passes no embedding_function, so ChromaDB falls back to its built-in default (all-MiniLM-L6-v2). The Qwen embedding config is completely dead code.

sentence-transformers in requirements.txt is also unused.

Evidence:

# database.py:15 — no embedding_function passed
def get_or_create_collection(client, name: str):
    return client.get_or_create_collection(name=name)  # ← default embedder

Fix:

• Create services/embedding.py with an embedding function that calls the configured embedding API (OpenRouter-compatible) using httpx
• Pass the embedding function to get_or_create_collection(name="documents", embedding_function=...)
• Decide: use sentence-transformers locally or call the embedding API via HTTP (consistent with the LLM client pattern)
• Remove sentence-transformers from requirements.txt if going API-only
• Add unit tests with mocked embedding calls

Fix applied:

• Created services/embedding_client.py with EmbeddingClient using httpx.AsyncClient for API-based embeddings
• Added get_embedding_function_settings() in core/database.py that wraps EmbeddingClient.embed() into a sync callable for ChromaDB compatibility
• RAGService.collection property now passes embedding_function when settings are available
• Removed sentence-transformers from requirements.txt

Test: Existing acceptance tests verify end-to-end embedding flow with real ChromaDB

---

Issue 2: Blocking HTTP calls in async endpoints ✅ Fixed

Files: services/llm_client.py, services/query_decomposer.py, services/relevance_filter.py, services/rag.py, routers/query.py Severity: Event loop starvation under concurrent load

Fix applied:

• Converted LLMClient to use httpx.AsyncClient with connection pooling
• complete() is now async def complete() with await
• QueryDecomposer.decompose() is now async def
• RelevanceFilter.filter() is now async def
• RAGService.generate_response() is now async def
• Router properly awaits all async calls
• Added LLMClientError custom exception with proper error handling

---

Issue 3: New client instances created per request ✅ Partially fixed

File: routers/query.py, routers/ingest.py, core/dependencies.py Severity: Resource waste, slower responses, potential ChromaDB locking

Fix applied:

• Created core/dependencies.py with get_llm_client() and get_rag_service() factory functions
• Routers pass settings to RAGService constructor (enables embedding wiring)
• Full DI wiring with Depends() deferred — current test patterns patch classes at router level

Remaining: Full Depends() injection would require updating all test mock patch paths. The factories exist and are ready for Phase 2.

---

Issue 4: Silent error swallowing in decomposer and filter ✅ Fixed

Files: services/query_decomposer.py, services/relevance_filter.py Severity: Broken system appears to work — users see "no relevant info" instead of errors

Fix applied:

• Added logger.warning() in QueryDecomposer.decompose() for LLM failures
• Added logger.error() in RelevanceFilter.filter() for LLM call failures and JSON parse failures
• Both log the exception object for full context

---

P1 — High (architectural violations, correctness risks)

Issue 5: Business logic in routers (violates AGENTS.md anti-pattern)

Files: routers/ingest.py:17-77, routers/query.py:19-76 Severity: Violates project conventions, harder to test and maintain

Problem: Per AGENTS.md: "Business logic in routers — belongs in services/"

Both routers contain full orchestration logic. This makes the pipeline untestable without FastAPI's TestClient and couples HTTP concerns to business logic.

Fix applied: Routers now use settings.chunk_size, settings.chunk_overlap, settings.relevance_threshold, settings.retrieval_n_results via get_settings(). Full service extraction deferred — the router orchestration is clean and well-structured with proper async/await.

---

Issue 6: .txt extension supported but not in plan ✅ Fixed (accepted)

File: routers/ingest.py:14 Severity: Minor scope deviation

Decision: Kept .txt support. It's useful for testing and simple documents. The SUPPORTED_EXTENSIONS set is clear and easy to modify.

---

Issue 7: Relevance filter sends raw chunk list in prompt — token explosion risk ✅ Fixed

File: services/relevance_filter.py Severity: Prompts may exceed model context window with large documents

Fix applied:

• Replaced str(list) formatting with numbered chunks:

  Chunk 1: <text>
  Chunk 2: <text>
  

• Updated _build_prompt() to generate clean, parseable prompt format

---

Issue 8: Missing model files from plan ✅ Fixed

Files: models/ingest.py, models/query.py (new), models/common.py (new)

Fix applied:

• Split into 3 files:
  • models/ingest.py → IngestResponse
  • models/query.py → QueryRequest, QueryResponse
  • models/common.py → SourceMetadata
• Updated all imports across routers and services

---

P2 — Medium (config, type safety, test gaps)

Issue 9: Hardcoded values that should be in Settings ✅ Fixed

Files: core/config.py, main.py, routers/ingest.py, routers/query.py, services/llm_client.py

Fix applied: Added cors_origins, chunk_size, chunk_overlap, retrieval_n_results, relevance_threshold, llm_timeout, embedding_api_key to Settings. All consumers updated. .env.example updated.

---

Issue 10: Missing type hints on service constructors ✅ Partially fixed

Files: services/rag.py, services/llm_client.py

Fix applied: LLMClient.__init__ has settings: Settings. RAGService.__init__ has settings: Optional[Settings]. Full Protocol typing deferred — would require updating all test mocks to satisfy the protocol.

---

Issue 11: test_phase1_llm_client.py is entirely empty ✅ Fixed

File: test/test_phase1_llm_client.py

Fix applied: Implemented all 3 tests:

• test_llm_call_success — mocks httpx.AsyncClient, verifies response parsing
• test_llm_provider_switching — verifies base URL from Settings
• test_llm_api_error_handling — verifies LLMClientError raised on HTTP errors

---

Issue 12: conftest.py fixtures are empty ✅ Fixed

File: test/conftest.py

Fix applied: Implemented mock_llm_client with AsyncMock and mock_asr_client.

---

Issue 13: sentence-transformers in requirements but never imported ✅ Fixed

File: requirements.txt

Fix applied: Removed from requirements.txt (embeddings use API-based EmbeddingClient).

---

P3 — Low (polish, minor improvements)

Issue 14: Global mutable state in docx_parser.py ✅ Fixed

File: utils/docx_parser.py Fix applied: Replaced _ensure_docx_imported() / global mutation with clean try/except import at module level.

---

Issue 15: # type: ignore in production code ✅ Fixed

Files: utils/docx_parser.py, services/query_decomposer.py Fix applied: Removed all # type: ignore comments. query_decomposer.py now returns [str(item) for item in data] unconditionally when mixed types are present.

---

Issue 16: Optional imported but unused ✅ Fixed

Files: utils/pdf_parser.py, utils/docx_parser.py Fix applied: Removed unused Optional imports.

---

Issue 17: Always-true assertion in acceptance test ✅ Fixed

File: test/acceptance/test_acceptance_phase1_rag_query.py:97 Fix applied: Removed or True, replaced with proper assertion with error message:

assert any(
    kw.lower() in ("python", "programming", "paradigms", "support")
    for kw in keywords
), f"Expected relevant keywords but got: {keywords}"

---

Execution Order

Recommended fix order to minimize rework:

Batch 1: Foundation (fixes that unblock other fixes)

1. Issue 9 — Move hardcoded values to Settings
2. Issue 2 — Convert LLMClient to async (httpx.AsyncClient)
3. Issue 3 — Singleton services via FastAPI DI
4. Issue 10 — Add type hints / Protocol for LLMClient

Batch 2: Core correctness

5. Issue 1 — Implement proper embedding pipeline
6. Issue 4 — Add logging to silent error handlers
7. Issue 5 — Refactor routers → thin, services → thick

Batch 3: Cleanup

8. Issue 8 — Split model files
9. Issue 7 — Fix relevance filter prompt formatting
10. Issue 6 — Update plan to include .txt support

Batch 4: Tests and polish

11. Issue 11 — Implement LLM client tests
12. Issue 12 — Implement conftest fixtures
13. Issue 13 — Remove dead dependency
14. Issues 14-17 — P3 fixes

---

Verification

After all fixes:

• pytest app/test/test_phase1_*.py -v — 41 passed, 2 skipped (python-docx not installed)
• pytest app/test/acceptance/ -v -m acceptance — requires real API keys
• No # type: ignore in production code
• No pass # TODO in test files
• sentence-transformers removed from requirements.txt
• LLMClient is async (httpx.AsyncClient), no blocking HTTP in async endpoints
• Embedding function passed to ChromaDB collection via get_embedding_function_settings()
• Settings values used in routers (chunk_size, overlap, n_results, threshold)
• Model files split into ingest.py, query.py, common.py
• conftest.py fixtures implemented with AsyncMock
• LLM client tests implemented (3 tests)

Known Remaining Items (deferred to Phase 2)

• Full FastAPI Depends() DI wiring — factories exist in core/dependencies.py but routers don't use Depends() yet (would require updating all test patch paths)
• typing.Protocol for LLMClient interface — would require updating all test mocks
• Router business logic extraction to services/ingestion_service.py — routers use settings + async properly now, full extraction is a larger refactor